akshaytayal commented on code in PR #13441:
URL: https://github.com/apache/gluten/pull/13441#discussion_r4384183972
##########
backends-velox/src/main/scala/org/apache/gluten/execution/HashAggregateExecTransformer.scala:
##########
@@ -504,7 +508,13 @@ abstract class HashAggregateExecTransformer(
val aggregateFunc = aggExpr.aggregateFunction
val childrenNodes = aggExpr.mode match {
case Partial | Complete =>
- aggregateFunc.children.toList.map(toExpressionNode)
+ VeloxIntermediateData.getRawInputs(aggregateFunc).toList.map {
+ case literal: Literal if rowConstructed =>
Review Comment:
Good question! `rowConstructed` is per operator, so it also affects
`Partial` functions when they share an operator with a row-constructed
`PartialMerge`. Spark does this for a single DISTINCT aggregate:
```sql
SELECT avg(a), collect_set(DISTINCT b) RESPECT NULLS FROM t
```
The distinct stage becomes `merge_avg(a), partial_velox_collect_set(distinct
b, false)`. `merge_avg` triggers row construction, so all inputs get projected,
including the `false` flag:
```
#0 = row(sum, count), #1 = b, #2 = false
-> merge_avg(#0), collect_set(#1, #2)
```
Velox only reads `ignoreNulls` from a constant, so with `#2` it keeps the
default and drops the null: we got `[2, 5]` instead of `[null, 2, 5]`. Keeping
the literal inline gives `collect_set(#1, false)`, which fixes it.
The test `collect_set RESPECT NULLS in an aggregate that also merges another
aggregate` covers this case. Happy to add a code comment there if helpful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
