plan: prune cor cols in gby items. #2568
Conversation
| if len(p.GroupByItems) > 0 { | ||
| for i := len(p.GroupByItems) - 1; i >= 0; i-- { | ||
| cols := expression.ExtractColumns(p.GroupByItems[i]) | ||
| if len(cols) == 0 { |
There was a problem hiding this comment.
Like group by '1' or group by a correlated column.
There was a problem hiding this comment.
Then we do not need to "selfUsedCols = append(selfUsedCols, cols...)"
plan/column_pruning.go
Outdated
| if len(cols) == 0 { | ||
| p.GroupByItems = append(p.GroupByItems[:i], p.GroupByItems[i+1:]...) | ||
| } | ||
| selfUsedCols = append(selfUsedCols, cols...) |
There was a problem hiding this comment.
if len(cols) == 0, should we save this line?
|
@shenli PTAL |
|
LGTM |
| for i := len(p.GroupByItems) - 1; i >= 0; i-- { | ||
| cols := expression.ExtractColumns(p.GroupByItems[i]) | ||
| if len(cols) == 0 { | ||
| p.GroupByItems = append(p.GroupByItems[:i], p.GroupByItems[i+1:]...) |
There was a problem hiding this comment.
How about just set p.GroupByItems[i] = expression.One? The line 103 can be removed.
The code would be mush simpler.
And add comments about why replace it to one.
There was a problem hiding this comment.
If we set i to One, it will cost more to encode.
There was a problem hiding this comment.
It's a very rare case, I think the saved cost doesn't worth the complexity.
There was a problem hiding this comment.
I don't think it adds the complexity.
There was a problem hiding this comment.
Writers always underestimate the complexity.
There was a problem hiding this comment.
Please the provide a benchmark result difference for the reduced group by items, and estimate the frequency this case will be run.
Then give a few example in our projects that have lower (performance Improvement * frequency) / complexity.
There was a problem hiding this comment.
I doesn't need a bench test. If it has massive group by items, it must be very inefficient.
There was a problem hiding this comment.
If you think that is complex, I think everything is complex, every optimization is complex, every sql should be written as best form by user.
plan/column_pruning.go
Outdated
| } | ||
| } | ||
| if len(p.GroupByItems) == 0 { | ||
| p.GroupByItems = []expression.Expression{expression.One} |
There was a problem hiding this comment.
Add comments about why use expression.One.
Rest LGTM
|
@zimulala PTAL |
|
LGTM |
|
PTAL @coocood |
|
LGTM |
cor cols is treated as constant in gby items.
@shenli @coocood @zimulala @tiancaiamao PTAL