-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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) #52489
planner: add newly created col for window projection (#52378) #52489
Conversation
Signed-off-by: ti-chi-bot <[email protected]>
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hawkingrei, hi-rustin 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:
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## release-7.1 #52489 +/- ##
================================================
Coverage ? 73.4204%
================================================
Files ? 1211
Lines ? 379890
Branches ? 0
================================================
Hits ? 278917
Misses ? 83143
Partials ? 17830 |
This is an automated cherry-pick of #52378
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.