-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
planner: add newly created col for window projection #52378
planner: add newly created col for window projection #52378
Conversation
Skipping CI for Draft Pull Request. |
Skipping CI for Draft Pull Request. |
/retest |
@@ -6502,6 +6502,14 @@ func (b *PlanBuilder) buildByItemsForWindow( | |||
} | |||
if col, ok := it.(*expression.Column); ok { | |||
retItems = append(retItems, property.SortItem{Col: col, Desc: item.Desc}) | |||
// We need to attempt to add this column because a subquery may be created during the expression rewrite process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This 3 lines is same as the under 3 lines from 6515 to 6521.
So should you please change those code more unified ~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. I have reordered the code to make them have the same order.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AilinKid, hawkingrei The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
In response to a cherrypick label: new pull request created to branch |
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <[email protected]>
In response to a cherrypick label: new pull request created to branch |
What problem does this PR solve?
Issue Number: close #42734
Problem Summary:
We will get a panic error during we execute the following SQL:
The problem happened in the
TryToGetChildProp
of theLogicalProjection
plan.// TryToGetChildProp will check if this sort property can be pushed or not. // When a sort column will be replaced by scalar function, we refuse it. // When a sort column will be replaced by a constant, we just remove it. func (p *LogicalProjection) TryToGetChildProp(prop *property.PhysicalProperty) (*property.PhysicalProperty, bool) { newProp := prop.CloneEssentialFields() newCols := make([]property.SortItem, 0, len(prop.SortItems)) for _, col := range prop.SortItems { idx := p.schema.ColumnIndex(col.Col) + switch expr := p.Exprs[idx].(type) { case *expression.Column: newCols = append(newCols, property.SortItem{Col: expr, Desc: col.Desc}) case *expression.ScalarFunction: return nil, false } } newProp.SortItems = newCols return newProp, true }
We cannot find the sort item from the projection's schema.
After I debugged it, I found that we will try to find the
Coulmn#14
in the projection's schema. But we don't have it.What changed and how does it work?
To understand this problem we need to take a look at the query plan after we fixed it :(
As you can see the window is partitioned by
Column#14
and it comes from theProjection_22
.Column#14
evaluates from the exist-subquery:When we built this subquery we found it is a correlated query because we used
temp_data.temperature
as the predicate.Then the problem came out from the
buildByItemsForWindow
, because we used the column from the semi-apply plan as our sort item during the expression rewrite then we forget to add this column into the projection's schema:So the fix is that we need to append this col to the top-level projection schema:
And also we need to avoid adding the same column twice, for example:
As you can see we partition the window by itself then we already have it in the projection's
schema. So we don't need to add it again.
The query plan looks like this
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.