aggfuncs: implement bit-or with new aggregation framework #6975
Conversation
executor/aggfuncs/func_bit_or.go
Outdated
| baseAggFunc | ||
| } | ||
|
|
||
| type result4BitOrUint64 struct { |
There was a problem hiding this comment.
s/result4BitOrUint64/partialResult4BitFunc/, so we can reuse it in other bit aggregate functions
executor/aggfuncs/builder.go
Outdated
| base := baseAggFunc{ | ||
| args: aggFuncDesc.Args, | ||
| ordinal: ordinal, | ||
| } |
There was a problem hiding this comment.
need to handle the function which has the distinct property.
There was a problem hiding this comment.
why bit-or need to care distinct property?
There was a problem hiding this comment.
consider this query: select bit_or(distinct a) from t; we only calculate the distinct values of column a in this kind of query.
There was a problem hiding this comment.
It doesn't matter, because bit_or(distinct a) = bit_or(a), bit_and same too, except bit_xor.
There was a problem hiding this comment.
please add a comment here to statement that function bitor no need to consider the distinct property
executor/aggfuncs/func_bit_or.go
Outdated
| baseAggFunc | ||
| } | ||
|
|
||
| type partialResult4BitFunc struct { |
There was a problem hiding this comment.
type partialResult4BitFunc uint64
executor/aggfuncs/func_bit_or.go
Outdated
| } | ||
|
|
||
| func (e *bitOrUint64) AllocPartialResult() PartialResult { | ||
| return PartialResult(&partialResult4BitFunc{}) |
There was a problem hiding this comment.
return PartialResult(&uint64)
executor/aggfuncs/func_bit_or.go
Outdated
| if err != nil { | ||
| return errors.Trace(err) | ||
| } | ||
|
|
executor/aggfuncs/func_bit_or.go
Outdated
| "github.com/pingcap/tidb/util/chunk" | ||
| ) | ||
|
|
||
| type bitOrUint64 struct { |
There was a problem hiding this comment.
type baseBitAggFunc struct{
baseAggFunc
}
type bitOrUint64 struct{
baseBitAggFunc
}thus baseBitAggFunc can be reused.
executor/aggfuncs/func_bit_or.go
Outdated
| func (e *bitOrUint64) UpdatePartialResult(sctx sessionctx.Context, rowsInGroup []chunk.Row, pr PartialResult) error { | ||
| p := (*partialResult4BitFunc)(pr) | ||
| for _, row := range rowsInGroup { | ||
| inputValue, isNull, err := e.args[0].EvalInt(sctx, row) |
There was a problem hiding this comment.
Should we wrap a cast as uint in typeInfer4BitFuncs,
or bit_or(varchar) may fail?
There was a problem hiding this comment.
bit_or( varchar ) will return 0.
There was a problem hiding this comment.
actually, we need to add a cast, consider this case:
drop table if exists t;
create table t(a decimal(10, 4));
insert into t values(12.2);
select bit_or(a) from (select * from t union all select * from t) tmp;TiDB(localhost:4000) > desc select bit_or(a) from (select * from t union all select * from t) tmp;
+--------------------------+------+----------------------------------------------+----------+
| id | task | operator info | count |
+--------------------------+------+----------------------------------------------+----------+
| StreamAgg_13 | root | funcs:bit_or(tmp.a) | 1.00 |
| └─Union_21 | root | | 20000.00 |
| ├─TableReader_24 | root | data:TableScan_23 | 10000.00 |
| │ └─TableScan_23 | cop | table:t, range:[-inf,+inf], keep order:false | 10000.00 |
| └─TableReader_27 | root | data:TableScan_26 | 10000.00 |
| └─TableScan_26 | cop | table:t, range:[-inf,+inf], keep order:false | 10000.00 |
+--------------------------+------+----------------------------------------------+----------+
6 rows in set (0.00 sec)The above StreamAgg_13 directly handles the original data instead of another aggregate operator's partial result, which is guaranteed to be uint64. This PR may failed on this query if we don't wrap a cast on it's parameter.
executor/aggfuncs/func_bit_or.go
Outdated
|
|
||
| type partialResult4BitFunc = uint64 | ||
|
|
||
| func (e *bitOrUint64) AllocPartialResult() PartialResult { |
There was a problem hiding this comment.
This can be a member function of *baseBitAggFunc.
There was a problem hiding this comment.
great suggestion.
There was a problem hiding this comment.
...I take back the last sentence. 😂
We should not make the result value to be a member of baseBitAggFunc , Because this will make The baseBitAggFunc to be Stateful.
Consider another scenario, If we have many groups to be aggregated, if the AggFunc is statefull, we have to create many aggFunc to handle this.
But if AggFunc is not statefull, we can only create one AggFunc and many partialResult4BitFunc, this will reduce go GC pressure.
( This is @zz-jason told me. Thanks very much~ )
executor/aggfuncs/func_bit_or.go
Outdated
| return PartialResult(new(partialResult4BitFunc)) | ||
| } | ||
|
|
||
| func (e *bitOrUint64) ResetPartialResult(pr PartialResult) { |
executor/aggfuncs/func_bit_or.go
Outdated
| *p = 0 | ||
| } | ||
|
|
||
| func (e *bitOrUint64) AppendFinalResult2Chunk(sctx sessionctx.Context, pr PartialResult, chk *chunk.Chunk) error { |
executor/aggfuncs/func_bit_or.go
Outdated
| @@ -0,0 +1,60 @@ | |||
| // Copyright 2018 PingCAP, Inc. | |||
There was a problem hiding this comment.
rename the filename as func_bitfuncs.go
executor/aggfuncs/func_bit_or.go
Outdated
|
|
||
| type baseBitAggFunc struct { | ||
| baseAggFunc | ||
| value uint64 |
There was a problem hiding this comment.
do not store the partial result into a aggregate function. aggregate functions should be stateless.
crazycs520
force-pushed
the
dev/agg-bit-or
branch
from
0ec3f50
to
0b00dc8
Compare
zz-jason
added
type/enhancement
executor/aggfuncs/builder.go
Outdated
| @@ -120,6 +120,15 @@ func buildGroupConcat(aggFuncDesc *aggregation.AggFuncDesc, ordinal int) AggFunc | |||
|
|
|||
| // buildCount builds the AggFunc implementation for function "BIT_OR". | |||
| func buildBitOr(aggFuncDesc *aggregation.AggFuncDesc, ordinal int) AggFunc { | |||
| // BIT_OR doesn't need to handle the distinct property. | |||
| switch aggFuncDesc.Args[0].GetType().Tp { | |||
There was a problem hiding this comment.
switch aggFuncDesc.Args[0].GetType().EvalType(){
case types.ETInt:
xxxx
}
|
/run-all-tests |
XuHuaiyu
added
status/LGT2
|
@crazycs520 This PR can be merged after the checks finish. |
What have you changed? (mandatory)
This PR implements bit-or with new aggregation framework
What are the type of the changes (mandatory)?
improvement
How has this PR been tested (mandatory)?
the existing test cases
Does this PR affect documentation (docs/docs-cn) update? (optional)
No
Refer to a related PR or issue link (optional)
#6952 #6852