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
aggregation, plan: split the presentation and evaluation layers of aggregation functions #5635
Conversation
…gregation functions
/run-all-tests |
Please add a few description for the PR. |
/run-all-tests tidb-test=pr/446 |
Cool |
expression/aggregation/descriptor.go
Outdated
// TODO: a.Args[0] = expression.WrapWithCastAsInt(ctx, a.Args[0]) | ||
} | ||
|
||
func (a *AggFuncDesc) calculateDefaultVale4Count(ctx context.Context, schema *expression.Schema) (types.Datum, bool) { |
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.
Vale
-> Value
.
expression/aggregation/descriptor.go
Outdated
} | ||
|
||
// GetEvaluator gets an evaluator according to the aggregation function signature. | ||
func (a *AggFuncDesc) GetEvaluator() Aggregation { |
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.
How about just GetAggFunc
or GetAggregation
?
We can distinguish it from Aggregation
by Desc
already.
plan/aggregation_push_down.go
Outdated
firstRow := aggregation.NewAggFunction(ast.AggFuncFirstRow, []expression.Expression{gbyCol.Clone()}, false) | ||
newAggFuncs = append(newAggFuncs, firstRow) | ||
firstRow := aggregation.NewAggFuncDesc(ast.AggFuncFirstRow, []expression.Expression{gbyCol.Clone()}, false) | ||
firstRow.TypeInfer(agg.ctx) |
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.
How about just combine the TypeInfer
and NewFuncDesc
for the complete model? Almost every NewFuncDesc
is followed by TypeInfer
.
LGTM |
plan/property_cols_prune.go
Outdated
@@ -39,6 +39,10 @@ func (p *LogicalSelection) preparePossibleProperties() (result [][]*expression.C | |||
return p.children[0].preparePossibleProperties() | |||
} | |||
|
|||
func (p *LogicalProjection) preparePossibleProperties() (result [][]*expression.Column) { | |||
return p.children[0].(LogicalPlan).preparePossibleProperties() |
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.
No need to do type assertion. Also, this is an irrelevant change?
… into dev/plan/aggregation
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.
LGTM
With this patch, I split the
Aggregation
interface to presentation and evaluation layers.AggFuncDesc
to describe an aggregation function, witch contains all the informations and APIs needed by planner.Aggregation
interface to represent the evaluator of a particularAggFuncDesc
object, irrelevant APIs are removed.AggFuncDesc.GetAggFunc()
to get the an instance ofAggregation
as the corresponding evaluator.After this PR, we are able to: