Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support order by limit optimization for non-sorted columns #18760

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

badboynt1
Copy link
Contributor

@badboynt1 badboynt1 commented Sep 12, 2024

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #17367

What this PR does / why we need it:

support order by limit optimization for non-sorted columns


PR Type

enhancement


Description

  • Removed a condition in the handleMessgaeFromTopToScan function that checked the sort order of a column, optimizing the order by limit for non-sorted columns.
  • Simplified the logic for processing messages in the query plan, potentially improving performance.

Changes walkthrough 📝

Relevant files
Enhancement
message.go
Optimize order by limit for non-sorted columns                     

pkg/sql/plan/message.go

  • Removed a condition that checks the sort order of a column in the scan
    node.
  • Simplified the logic for handling messages from top to scan nodes.
  • +0/-3     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Potential Performance Impact
    Removal of the sort order check might lead to unexpected behavior or performance issues for certain queries involving non-sorted columns.

    @matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label Sep 12, 2024
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Add a comment explaining the removal of the sort order check

    Consider adding a comment explaining why the check for the sort order of the column
    was removed. This will help future maintainers understand the reasoning behind this
    change.

    pkg/sql/plan/message.go [72-73]

     if orderByCol.Col.RelPos != scanNode.BindingTags[0] {
         return
     }
    +// The check for column sort order was removed to support order by limit optimization for non-sorted columns
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a comment to explain the removal of the sort order check improves maintainability by providing context for future developers, helping them understand the rationale behind the change.

    8
    Possible issue
    Add a check to ensure the column is sorted before applying the optimization

    Consider adding a check to ensure that the column is actually sorted before applying
    the optimization. This will prevent potential issues with unsorted columns.

    pkg/sql/plan/message.go [64-73]

     orderByCol, ok := node.OrderBy[0].Expr.Expr.(*plan.Expr_Col)
     if !ok {
         return
     }
     scanNode := builder.qry.Nodes[scanID]
     if len(scanNode.OrderBy) != 0 {
         panic("orderby in scannode should be nil!")
     }
     if orderByCol.Col.RelPos != scanNode.BindingTags[0] {
         return
     }
    +if !isColumnSorted(scanNode.TableDef, orderByCol.Col.ColPos) {
    +    return
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion adds a check to ensure that the column is sorted, which could prevent potential issues with unsorted columns. However, it may not align with the intended simplification of the logic in the PR.

    7

    @mergify mergify bot merged commit afc9629 into matrixorigin:1.2-dev Sep 12, 2024
    18 checks passed
    @badboynt1 badboynt1 deleted the orderby1.2 branch September 13, 2024 01:42
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants