feat: native RANGE window frames with explicit offset on DATE ORDER BY#4974
feat: native RANGE window frames with explicit offset on DATE ORDER BY#49740lai0 wants to merge 7 commits into
Conversation
|
Fixed CI |
andygrove
left a comment
There was a problem hiding this comment.
Following up on my earlier look, I need to walk back the approval. I approved prematurely before checking the boundary overflow behavior, and there is a Spark-compatibility gap here. (Reviewed with LLM assistance.)
The core of the change is good: coercing the integer offset to IntervalDayTime so Date32 + IntervalDayTime runs natively is correct for all realistic inputs, since Date32 is day-unit and the direction is carried by the bound variant. The twelve checkSparkAnswerAndOperator cases are thorough. My concern is only the overflow edge.
I traced both sides:
- Spark computes the DATE RANGE boundary via
DateAdd(WindowEvaluatorFactoryBase), andDateAdd.nullSafeEvalis plainstart + dInt addition with silent wraparound. It is notMath.addExactand not ANSI-gated, so it never throws. On overflow it produces a wrapped date and compares normally. - Comet/DataFusion computes the boundary via
ScalarValue::add(Date32, IntervalDayTime), which routes to arrow'sdate_opusingadd_day_time(...). That returnsErron overflow (arrow date arithmetic is always checked, even through the wrapping entry point), and the window frame collapses the bound to the partition edge. This is the same mechanism the #4987 author documented for DECIMAL.
So on boundary overflow the two engines produce different frames. This is the identical gap as the DECIMAL sibling #4987, and unlike that PR this one does not document it.
Per our compatibility contract, a case that diverges from Spark should not run natively by default. It needs to be either made compatible or marked Incompatible and gated behind spark.comet.operator.<name>.allowIncompatible, falling back to Spark by default (following the CometDataWritingCommand precedent).
I do want to be fair about the practical likelihood: overflowing Date32 requires a RANGE offset of roughly 2.1 billion days (~5.88 million years) from a realistic date, so this is a far more remote corner than the DECIMAL case. If the team decides that is remote enough to accept, then at minimum this PR should document the divergence (as #4987 does) and add a Date32-boundary test that pins the behavior. But the default of running an incompatible edge natively with no note is what I would like changed.
I have filed #5022 to track the overflow divergence for both DATE and DECIMAL so the two PRs can reference it and be resolved consistently. Since this and #4987 edit the same files, they are best reviewed and decided as a pair.
Requesting changes so we settle the overflow handling before this merges. Everything else looks ready, and thanks for the clean implementation and strong test coverage.
Which issue does this PR close?
Part of #4834 (DATE ORDER BY only; DECIMAL remains a follow-up)
Rationale for this change
Native windows used to fall back to Spark for
RANGEframes with an explicitPRECEDING/FOLLOWINGoffset whenORDER BYwasDATE. Spark ships that offset as an integer literal, but arrow-arith rejectsDate32 + Int32and requires anIntervalRHS.This PR coerces the offset to
IntervalDayTimewhen the ORDER BY column isDate32, so boundary math usesDate32 + IntervalDayTimeand the DATE fallback can be removed.UNBOUNDED/CURRENT ROWalready ran natively; DECIMAL still falls back.What changes are included in this PR?
Int32/Int64RANGE offsets toIntervalDayTimeforDate32ORDER BY.DateTypeRANGE fallback; keep the DECIMAL fallback.CometWindowExecSuite(offsets, duplicate dates, nulls, unbounded baseline).How are these changes tested?
./mvnw test -Dtest=none -Dsuites="org.apache.comet.exec.CometWindowExecSuite"