Skip to content

feat: native RANGE window frames with explicit offset on DATE ORDER BY#4974

Open
0lai0 wants to merge 7 commits into
apache:mainfrom
0lai0:feat-4834-range-offset-date-order-by
Open

feat: native RANGE window frames with explicit offset on DATE ORDER BY#4974
0lai0 wants to merge 7 commits into
apache:mainfrom
0lai0:feat-4834-range-offset-date-order-by

Conversation

@0lai0

@0lai0 0lai0 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

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 RANGE frames with an explicit PRECEDING / FOLLOWING offset when ORDER BY was DATE. Spark ships that offset as an integer literal, but arrow-arith rejects Date32 + Int32 and requires an Interval RHS.
This PR coerces the offset to IntervalDayTime when the ORDER BY column is Date32, so boundary math uses Date32 + IntervalDayTime and the DATE fallback can be removed. UNBOUNDED / CURRENT ROW already ran natively; DECIMAL still falls back.

What changes are included in this PR?

  • Native planner : coerce Int32 / Int64 RANGE offsets to IntervalDayTime for Date32 ORDER BY.
  • Scala serde : drop the DateType RANGE fallback; keep the DECIMAL fallback.
  • Tests : 7 cases in CometWindowExecSuite (offsets, duplicate dates, nulls, unbounded baseline).

How are these changes tested?

./mvnw test -Dtest=none -Dsuites="org.apache.comet.exec.CometWindowExecSuite"

@0lai0

0lai0 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Fixed CI
removed the stale expect_fallback for the DATE RANGE case in window_functions.sql.
It now runs natively and is validated against Spark.

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @0lai0. Would be good to get a review from @comphead as well.

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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), and DateAdd.nullSafeEval is plain start + d Int addition with silent wraparound. It is not Math.addExact and 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's date_op using add_day_time(...). That returns Err on 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants