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

[0lai0]

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]

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]

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

[andygrove]

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.

[sunchao]

Summary

• Prior state and problem: DATE RANGE frames with integer offsets fell back because Arrow cannot directly combine Date32 and integer offsets.
• Design approach: Convert offsets to day-only IntervalDayTime scalars and remove the DATE fallback.
• Correctness / compatibility analysis: Ordinary cases matched Spark across ASC/DESC, null ordering, duplicate dates, and both ANSI modes. The existing P2 overflow concern in #5022 remains substantiated. For dates 2024-01-01, 2024-01-02, and 2024-01-05, COUNT(*) OVER (ORDER BY d RANGE BETWEEN CURRENT ROW AND 2147483647 FOLLOWING) returns [0,0,0] in Spark, while the pinned DataFusion frame calculation includes [3,2,1] rows. This duplicates the existing changes-requested review, so it is not added as a new finding.
• Key design decisions: Type resolution and offset conversion happen during planning. The small helper introduces no additional per-row conversion or substantial abstraction complexity.
• Implementation sketch: Resolve the ordering datatype, coerce both explicit bounds, retain existing unsupported-case fallbacks, and update tests and documentation.
• Behavioral changes worth calling out: Integer DATE offsets now execute natively by default. DECIMAL and nonnumeric interval offsets retain their fallbacks.
• Suggested improvements: Resolve the existing blocker with Spark-compatible boundary arithmetic or restore fallback for incompatible cases, with regression coverage.

Reviewed all six changed files against base d075ab326e7601ffb65dc3d156ee4b00362ec06b, at full head 35460c7941271237d8d58e47b063594563b0c76a. No additional introduced P1/P2 issues found within this review.

Routed skills: review-comet-pr and applicable compatibility checks from audit-comet-expression, using this head’s .claude/skills/ paths because .ai/skills/ is absent. Read AGENTS.md, contributor guidance, existing reviews, comments, and threads.

Exact-head CI: 60 successful, 9 skipped, 1 failed. The Spark 3.5 execution log confirms all seven added tests passed. The macOS Spark 4.0 scan job failed with JVM SIGBUS; its connection to this PR is unsubstantiated.

Validation: Compared Spark sources for 3.4.3, 3.5.8, 4.0.2, 4.1.2, and 4.2.0. A disposable harness using the pinned Arrow 58.3.0/DataFusion 54.1.0 dependencies matched Spark 4.1.3 on all 480 ordinary frame-count comparisons and reproduced boundary divergences. Local validation exercised dependency-level frame logic, not full Comet integration. No full Comet build, JVM suite, or performance benchmark was run locally. The checkout remains unchanged.

[dwsmith1983]

This has been waiting since the 2026-07-24 review about boundary overflow. Do you plan to come back to it? If not, it could be closed and the fallback kept until someone picks it up.

[sunchao]

Summary

• Prior state and problem: DATE RANGE frames with explicit integer offsets fell back because Arrow does not support Date32 + Int32 arithmetic.
• Design approach: Convert DATE offsets to day-only IntervalDayTime values during native planning and remove the DATE fallback.
• Correctness / compatibility analysis: Ordinary cases matched Spark. The existing P2 overflow blocker in #5022 remains unresolved. For dates 2024-01-01, 2024-01-02, and 2024-01-05, COUNT(*) OVER (ORDER BY d RANGE BETWEEN CURRENT ROW AND 2147483647 FOLLOWING) returns [0,0,0] in Spark, while the pinned native frame calculation includes [3,2,1] rows. This reproduces in both ANSI modes and duplicates the existing review concern.
• Key design decisions: A small shared helper converts both bounds during planning, with checked narrowing for Int64. It adds no per-row conversion layer or substantial abstraction complexity.
• Implementation sketch: Resolve the ordering datatype, coerce explicit bounds, remove the Scala DATE guard, and update tests and compatibility documentation.
• Behavioral changes worth calling out: Integer DATE offsets now execute natively by default. DECIMAL, nonnumeric offsets, and other existing unsupported cases retain their fallbacks.
• Suggested improvements: Resolve the existing blocker with Spark-compatible boundary arithmetic or restore fallback for incompatible cases, with regression coverage.

Reviewed all six changed files in the full diff from base d075ab326e7601ffb65dc3d156ee4b00362ec06b to head 35460c7941271237d8d58e47b063594563b0c76a. The PR is not a draft. No additional introduced P1/P2 issues found within this review.

Routed skills: review-comet-pr and applicable audit-comet-expression checks, using this head’s .claude/skills/ equivalents because .ai/skills/ is absent. Read repository guidance and all supplied reviews, comments, and threads.

Exact-head CI: 60 successful checks, nine skipped, one failed. CI used the PR merge ref against the specified base. All seven added tests passed in the Spark 3.5 execution job. The macOS Spark 4.0 scan job failed with JVM SIGBUS; a connection to this change is unsubstantiated.

Validation: Compared upstream Spark sources for 3.4.3, 3.5.8, 4.0.2, 4.1.2, and 4.2.0. Reran a disposable Arrow 58.3.0/DataFusion 54.1.0 frame probe against Spark 3.5.9: all 480 ordinary comparisons matched across ordering directions, null ordering, duplicate dates, frame shapes, and ANSI modes. Boundary cases reproduced the existing divergence. Local validation exercised dependency-level frame logic, not full Comet integration. No full Comet build, JVM suite, or performance benchmark was run locally. Project code remains unchanged.

[kazuyukitanimura]

@0lai0 Do you mind resolving the conflicts please?

[andygrove]

Thanks @0lai0, this addresses my earlier concern about the overflow divergence.

I checked the new guard. The 2144550751 limit is Int.MaxValue minus the epoch day of 9999-12-31, so no date between 0001-01-01 and 9999-12-31 can wrap in either direction. The Long.MinValue and Int.MinValue cases fall back, and both bounds are checked. I also read the pinned DataFusion frame code. An overflowing bound collapses to the partition edge there. That agrees with Spark for offsets at or below the limit, because Spark's unwrapped bound lies beyond every row. The fallback test and the largest-safe-offset test pin both sides.

The remaining divergence for dates outside Spark's supported range is documented in operators.md, which seems right since it depends on the data.

Since this touches the serde and the planner, it would be good to get a green run with a run-spark-* label before merging.