executor, util: wrap cast upon the args for AggFunction #7180
Conversation
XuHuaiyu
added
type/enhancement
|
PTAL @zz-jason |
|
/run-all-tests |
| @@ -825,11 +825,43 @@ func (b *executorBuilder) buildHashJoin(v *plan.PhysicalHashJoin) Executor { | |||
| return e | |||
| } | |||
|
|
|||
| // wrapCastForAggArgs wraps the args of an aggregate function with a cast function. | |||
| func (b *executorBuilder) wrapCastForAggArgs(funcs []*aggregation.AggFuncDesc) { | |||
There was a problem hiding this comment.
We'd better make implement a function like InferArgumentType for AggFuncDesc, and call that function inside this routine:
for _, aggFuncDesc := range funcs {
aggFuncDesc.InferArgumentType()
}There was a problem hiding this comment.
I think current function name may be clearer,
since this is not a real type infer.
Type infer for agg function has been done in AggFuncDesc.typeInfer.
This function is more like a supplementary behavior for type infer.
util/chunk/row.go
Outdated
| // we should set it to the real fraction length of the decimal value, | ||
| // if not, the d.Frac will be set to MAX_UINT16 which will cause unexpected BadNumber error | ||
| // when encoding. | ||
| if tp.Decimal == -1 { |
util/chunk/row.go
Outdated
| // If tp.Decimal is unspecified(-1), | ||
| // we should set it to the real fraction length of the decimal value, | ||
| // if not, the d.Frac will be set to MAX_UINT16 which will cause unexpected BadNumber error | ||
| // when encoding. |
There was a problem hiding this comment.
Please make the block comment looks like a "block", you can take "llvm", "impala" or any other projects as examples:
|
I think the integration tests are filed because the newer implemented aggregate function |
|
/run-common-test tidb-test=pr/595 |
1 similar comment
|
/run-common-test tidb-test=pr/595 |
executor/builder.go
Outdated
| // buildProjBelowAgg builds a ProjectionExec below AggregationExec. | ||
| // If all the args of `aggFuncs`, and all the item of `groupByItems` | ||
| // are columns or constants, we do not need to build the `proj`. | ||
| func (b *executorBuilder) buildProjBelowAgg(aggFuncs []*aggregation.AggFuncDesc, groupByItems []expression.Expression, src Executor) Executor { | ||
| hasScalarFunc := false | ||
| // If the mode if Partial2Mode or CompleteMode, we do not need to wrap cast upon the args, | ||
| // since the types of the args are already the expected. | ||
| if len(aggFuncs) > 0 && (aggFuncs[0].Mode == aggregation.Partial1Mode || aggFuncs[0].Mode == aggregation.CompleteMode) { |
There was a problem hiding this comment.
I think len(aggFuncs) > 0 is enough, aggFuncs[0].Mode == aggregation.Partial1Mode || aggFuncs[0].Mode == aggregation.CompleteMode is guaranteed to be true in this procedure.
|
PTAL @winoros |
|
/run-common-test tidb-test=pr/595 |
What have you changed? (mandatory)
To support all the types for new aggregation framework, we need to wrap cast upon the args of SUM/AVG/BIT_FUNCS/GROUP_CONCAT.
What is the type of the changes? (mandatory)
How has this PR been tested? (mandatory)
The exist tests.
Does this PR affect documentation (docs/docs-cn) update? (mandatory)
no
Does this PR affect tidb-ansible update? (mandatory)
no
Does this PR need to be added to the release notes? (mandatory)
no
Refer to a related PR or issue link (optional)
#6952
Benchmark result if necessary (optional)
Add a few positive/negative examples (optional)