sunchao commented on code in PR #5972:
URL: https://github.com/apache/datafusion-comet/pull/5972#discussion_r4315798859
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -818,6 +837,34 @@ object QueryPlanSerde extends Logging with CometExprShim
with CometTypeShim {
}
val fn = aggExpr.aggregateFunction
+ if (aggExpr.mode == Partial || aggExpr.mode == Complete) {
+ // Raw-input aggregates can skip arguments (FILTER, null-skipping, or
state-dependent
+ // updates). Final/PartialMerge only read buffers, so their original
children are irrelevant.
+ var pending = fn.children.toList
+ while (pending.nonEmpty) {
+ val child = pending.head
+ pending = pending.tail
+ child match {
+ case nextDay: NextDay
+ if CometNextDay.evaluationMaskName(nextDay).isDefined &&
+ (aggExpr.filter.isDefined || fn.isInstanceOf[First] ||
fn.isInstanceOf[Last]) =>
Review Comment:
[P2] Keep unfiltered `Last` eligible for native execution. With ANSI enabled
and dynamic weekday columns, both `last(next_day(d,dow), false)` and
`last(next_day(d,dow), true)` now lose their native partial and final
aggregates. Unlike `First`, Spark’s `Last.updateExpressions` evaluates its
child on every row: directly when `ignoreNulls=false`, and through
`child.isNull` when it is true. There is therefore no skipped-child mask to
preserve here. This unnecessarily disables acceleration for previously
supported queries. Remove the unconditional `Last` arm while retaining
protection for aggregate `FILTER` and genuinely skipped arguments, and add
admission controls for both settings.
Evidence: Reproduced on Spark 4.1.3/JDK 17 with ANSI enabled, AQE disabled,
and whole-stage codegen both enabled and disabled. A Parquet table containing
`(DATE '2024-01-01', 'Monday')` returns the same date for both LAST queries,
but this head has zero `CometHashAggregateExec` nodes versus two with the
exact-base `CometExecRule` and `QueryPlanSerde` controls.
`max(next_day(d,dow))` retains two native aggregate nodes on both. An
invalid-weekday control throws in Spark and both Comet variants for both LAST
settings, confirming that LAST consumes the argument. Spark’s eager LAST update
expressions were checked across all five supported versions. Probe results are
recorded in `/tmp/review5972-oct19-n03j0mg6/head-tests.log` and
`base-probes.log`.
--
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]
