sunchao commented on code in PR #4951:
URL: https://github.com/apache/datafusion-comet/pull/4951#discussion_r4105247668
##########
spark/src/main/scala/org/apache/comet/serde/aggregates.scala:
##########
@@ -970,6 +970,7 @@ object AggSerde {
case FloatType | DoubleType => true
case _: DecimalType => true
case DateType | TimestampType => true
+ case dt if isTimeType(dt) => true
Review Comment:
[P2] Preserve fallback for sliding TIME minima until the accumulator is
fixed. `CometWindowExec.windowExprToProto` also uses this predicate, so this
addition enables `MIN(t) OVER (ORDER BY id ROWS BETWEEN 1 PRECEDING AND CURRENT
ROW)` for TIME values. With successive values `12:34:56.123456`, `NULL`,
`NULL`, and `00:00:00`, the third row should return `NULL`. The native window
evaluator instead returns `12:34:56.123456`: DataFusion's
`SlidingMinAccumulator` removes the expired non-null value but does not clear
its cached minimum when the remaining frame contains only nulls. This silently
changes previously correct Spark fallback results. Please either retain
fallback for affected TIME windows or fix the accumulator before enabling them,
with a regression test for this transition.
Evidence: Ran `cargo run --offline --manifest-path
/tmp/pr4951-review-1790344367-window/Cargo.toml` using DataFusion 54.1.0 and
Arrow 58.4.0, matching the PR's dependency versions.
`SlidingAggregateWindowExpr` produced minima `[45296123456000, 45296123456000,
45296123456000, 0]` for the four-row input. MAX correctly produced
`[45296123456000, 45296123456000, NULL, 0]`. Running the equivalent SQL over
Parquet-backed `make_time` values on Spark 4.1.3 returned NULL for both
aggregates at row 3. Source tracing confirms the new predicate admits this
window, and `PhysicalPlanner::process_agg_func` selects DataFusion's built-in
`min`. Before this PR, the predicate rejected TIME and retained Spark execution.
--
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]
