aggfuncs: partially implement "AVG" #6951
Conversation
|
/run-all-tests |
zz-jason
added
status/all tests passed
type/enhancement
| // All the AggFunc implementations for "COUNT" are listed here. | ||
| // All the AggFunc implementations for "SUM" are listed here. | ||
| // All the AggFunc implementations for "AVG" are listed here. | ||
| _ AggFunc = (*avgOriginal4Decimal)(nil) |
There was a problem hiding this comment.
means this aggregate function is in the partial 1 or complete mode and it's input is the data from the child of logical aggregate operator, not the partial result of other aggregate function.
There was a problem hiding this comment.
avg4Decimal maybe confusing with avgPartial4Decimal, I prefer the old name.
executor/aggfuncs/func_avg.go
Outdated
|
|
||
| // All the AggFunc implementations for "AVG" are listed here. | ||
| var ( | ||
| _ AggFunc = (*avgOriginal4Decimal)(nil) |
There was a problem hiding this comment.
Duplicate with the code in aggfuncs.go.
executor/aggfuncs/aggfuncs.go
Outdated
| _ AggFunc = (*avgOriginal4Float64)(nil) | ||
| _ AggFunc = (*avgPartial4Float64)(nil) | ||
|
|
||
| _ AggFunc = (*avgOriginal4Float32)(nil) |
There was a problem hiding this comment.
If we wrap a castAsReal in typeInfer4Avg when the type of the arg is TypeFloat, this function signature may be removed?
There was a problem hiding this comment.
Yes. But in order to be compatible with mysql, we can not wrap a cast as real when the input parameter is a column and its type is float32
There was a problem hiding this comment.
avg(float) returns double in MySQL.
| // All the AggFunc implementations for "COUNT" are listed here. | ||
| // All the AggFunc implementations for "SUM" are listed here. | ||
| // All the AggFunc implementations for "AVG" are listed here. | ||
| _ AggFunc = (*avgOriginal4Decimal)(nil) |
| case aggregation.CompleteMode, aggregation.Partial1Mode: | ||
| switch aggFuncDesc.Args[0].GetType().Tp { | ||
| case mysql.TypeNewDecimal: | ||
| if aggFuncDesc.HasDistinct { |
There was a problem hiding this comment.
Can we check it at the beginning of this function?
There was a problem hiding this comment.
No, we only need to check this when the aggregate function is on CompleteMode or Partial1Mode
executor/aggfuncs/func_avg.go
Outdated
| inputSum, isNull, err := e.args[1].EvalReal(sctx, row) | ||
| if err != nil { | ||
| return errors.Trace(err) | ||
| } else if isNull { |
executor/aggfuncs/func_avg.go
Outdated
| inputCount, isNull, err := e.args[0].EvalInt(sctx, row) | ||
| if err != nil { | ||
| return errors.Trace(err) | ||
| } else if isNull { |
|
LGTM |
| @@ -58,6 +59,42 @@ func buildSum(aggFuncDesc *aggregation.AggFuncDesc, ordinal int) AggFunc { | |||
|
|
|||
| // buildCount builds the AggFunc implementation for function "AVG". | |||
There was a problem hiding this comment.
buildCount here may be a type?
What have you changed? (mandatory)
partially implement the
AVGfunction under the new aggregate function framework, only implemented the followingAVGfunctions:the following
AVGfunctions are not implemented:What are the type of the changes (mandatory)?
How has this PR been tested (mandatory)?
Does this PR affect documentation (docs/docs-cn) update? (optional)
No
Refer to a related PR or issue link (optional)
To #6952. This is the partially supported
AVGfunctions I mentioned in #6852