Skip to content

feat: native RANGE window frames with explicit offset on " DECIMAL " ORDER BY#4987

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

feat: native RANGE window frames with explicit offset on " DECIMAL " ORDER BY#4987
0lai0 wants to merge 4 commits into
apache:mainfrom
0lai0:feat-4834-range-offset-decimal-order-by

Conversation

@0lai0

@0lai0 0lai0 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #4834 (DECIMAL ORDER BY only; DATE is handled in #4974)

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 DECIMAL. The original reason was that Spark decimal arithmetic widens precision on +/-, so the computed boundary (e.g. Decimal(11,0)) no longer matched the current value's precision (e.g. Decimal(10,0)) and DataFusion's comparator failed with "Uncomparable values".

That mismatch was fixed upstream in apache/datafusion#22174 (closes #22113): ScalarValue::partial_cmp now ignores precision for Decimal128 when the scales match. The fix ships in DataFusion 54.0.0, which Comet already pins, so DECIMAL RANGE boundary arithmetic runs correctly on the native side with no planner changes needed. This PR just removes the now-stale Scala fallback. UNBOUNDED / CURRENT ROW already ran natively; DATE still falls back.

What changes are included in this PR?

  • Scala serde: drop the DecimalType RANGE fallback; keep the DATE fallback.
  • No native planner change needed (upstream DataFusion already handles the boundary comparison).
  • Docs: remove DECIMAL from the RANGE fallback list in operators.md / roadmap.md, and document a Decimal128 near-max-precision overflow difference under "Known differences".
  • Tests: 12 cases in CometWindowExecSuite (bounds combinations, duplicate values, nulls, DESC ordering, zero scale, high precision/scale, negative values crossing zero, fractional offset, DECIMAL(38,0) max precision).

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 test in window_function.sql

@andygrove

Copy link
Copy Markdown
Member

Thanks @0lai0, this is a clean removal and I appreciate that you tracked down the exact upstream fix. (Reviewed with LLM assistance.)

I confirmed the core claim: the comparator fix from apache/datafusion#22174 shipped in DataFusion 54.0.0, and we pin 54.1.0 in native/Cargo.toml, so it's present. The precision-ignoring-when-scales-match comparison is also the right semantics for RANGE boundaries, since Spark's decimal +/- widens precision but keeps scale, so boundary and current value share a scale and differ only in precision. The updated comment explaining the DATE-vs-DECIMAL coercion difference is accurate and helpful. The twelve checkSparkAnswerAndOperator cases are thorough, and I like that the DECIMAL(38,0) case is deliberately built to stay off the overflow edge.

The overflow case needs to change from default-native to opt-in. Right now the DECIMAL RANGE path runs natively by default, and the near-max-precision overflow divergence is captured only in prose in operators.md. Under our compatibility contract that's the wrong default. The native result diverges from Spark there, and the ANSI flavor is the concerning one: Spark throws on the overflow, but Comet would silently return a partition-edge value, which turns an error into a wrong answer. A behavior that isn't Spark-compatible shouldn't run by default.

Two ways to resolve it, in order of preference:

  1. Make it actually compatible (best). Fix the native boundary overflow to match Spark: NULL under non-ANSI, throw under ANSI. Then it stays Compatible with no opt-in and no caveat. This probably means an upstream DataFusion change, so it's more work, but it's the real fix.
  2. Mark it Incompatible and gate on opt-in. Default falls back to Spark (exact), and a user sets spark.comet.operator.<name>.allowIncompatible=true to get the native path with the documented caveat. This follows the existing CometDataWritingCommand precedent, which returns Incompatible(Some(...)) and only runs native when the user opts in.

Documenting it as a known difference while still running native by default isn't something we can ship. Whichever route you pick, a test that actually pins the chosen behavior (rather than avoiding the edge) would be good to have.

Two smaller points:

  • CI hasn't run on the branch yet, the fork workflow is waiting for maintainer approval. I'll get that going.
  • Coordinate with feat: native RANGE window frames with explicit offset on DATE ORDER BY #4974. Both PRs edit CometWindowExec.scala and window_functions.sql, one removing the DATE fallback and one removing DECIMAL. Whichever lands second will need a rebase, and the final comment should describe both DATE and DECIMAL as native rather than leaving a stale "still falls back" line. Worth reviewing the two together.

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