[GLUTEN-4763][VL] Add RewriteTypedImperativeAggregate rule for collect_list #4764
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Run Gluten Clickhouse CI |
liujiayi771
force-pushed
the
rewrite-typed-imperative-agg
branch
from
dbaabba to
744dd73
Compare
|
Run Gluten Clickhouse CI |
liujiayi771
force-pushed
the
rewrite-typed-imperative-agg
branch
from
744dd73 to
f630c5b
Compare
|
Run Gluten Clickhouse CI |
|
cc @PHILO-HE, thanks. |
liujiayi771
force-pushed
the
rewrite-typed-imperative-agg
branch
from
f630c5b to
0bfd92b
Compare
|
Run Gluten Clickhouse CI |
liujiayi771
force-pushed
the
rewrite-typed-imperative-agg
branch
from
0bfd92b to
908cfa2
Compare
|
Run Gluten Clickhouse CI |
PHILO-HE
approved these changes
Feb 26, 2024
There was a problem hiding this comment.
Looks good to me. Thanks so much for your work!
I note collect_list/set were excluded for window operator. Maybe, they can be removed now. It can be done in a separate pr. Thanks!
https://github.com/oap-project/gluten/blob/bda60be4c44621c3722ecfbe55e98fd22f06b04a/cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc#L645
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
For some typed imperative aggregate functions, like
collect_list/collect_set, Spark will make agg buffer stored asBinaryTypein partial/ partial merge phase, even though these two functions' output isArrayType. Gluten has no such special handling. For Velox backend, agg function's raw data type is used for agg buffer in partial/partial merge. Thus, there will be a type mismatch issue in the latter shuffle operator, as Gluten plan inherits attributes from its corresponding Spark plan. Forcollect_list/collect_set, shuffle expectsBinaryType, but getsArrayType.So, we need a rule
RewriteTypedImperativeAggregateto rewritecollect_listandcollect_setfor Velox backend. Referenced the implementation in #2669.Velox does not register the companion functions for
collect_set, to enablecollect_set, modifications need to be made in Velox.Currently, there are still some issues, such as the semantics of null, where Spark and Velox have many differences. At present,
collect_listsupports ignoring null values, but when all inputs are null, Spark returns an empty array, while Velox returns null.collect_setdoes not yet support ignoring null values, and there is the same problem when all inputs are null. Adaptations on the Velox side are still needed.How was this patch tested?
collect_listinVeloxAggregateFunctionsSuite.distinct functionsVeloxAggregateFunctionsSuite.collect_list null inputs