feat: native RANGE window frames with explicit offset on " DECIMAL " ORDER BY by 0lai0 · Pull Request #4987 · apache/datafusion-comet

0lai0 · 2026-07-21T07:48:53Z

Which issue does this PR close?

Closes #4834 (DECIMAL ORDER BY only; DATE was handled in #4974). Part of #5022 (the DECIMAL side of the boundary overflow).

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 was first released in DataFusion 54.0.0, so DECIMAL RANGE boundary arithmetic runs correctly on the native side with no planner changes needed, and this PR removes the blanket Scala fallback.

One shape still diverges, so it keeps falling back. Spark builds a DECIMAL bound with DecimalAddNoOverflowCheck (WindowEvaluatorFactoryBase), which keeps the exact value: it neither returns NULL nor throws. DataFusion rescales both operands to the larger scale and adds them as an i128, and when that overflows it takes the partition edge as the bound, which also takes in the NULL keys sorted at that edge. For example, DECIMAL(38,0) values [0, N, NULL] with N = 9e37 and ORDER BY d ASC NULLS LAST RANGE BETWEEN N PRECEDING AND N FOLLOWING give [2,2,1] in Spark and [2,3,1] natively. The bound can only overflow when the largest value of the ORDER BY type plus the offset exceeds 2^127 - 1 unscaled, which only DECIMAL(38, s) with an offset above about 7.01e37 unscaled reaches.

What changes are included in this PR?

Scala serde: replace the blanket DecimalType RANGE fallback with isDecimalRangeOffsetSafe, which falls back only when the largest value of the ORDER BY type plus the offset can exceed 2^127 - 1 unscaled. Every DECIMAL(p, s) with p <= 37, and DECIMAL(38, s) with any offset below that limit, now runs natively.
No native planner change needed (upstream DataFusion already handles the boundary comparison).
Docs: remove DECIMAL from the remaining-work list in roadmap.md, and replace the DECIMAL RANGE entry in the fallback list in operators.md with one for offsets whose bound can overflow Decimal128.
SQL fixture: the DECIMAL ORDER BY DESC case in window_functions.sql now runs natively, and its comment no longer says the DATE case falls back (feat: native RANGE window frames with explicit offset on DATE ORDER BY #4974 removed that fallback).
Tests in CometWindowExecSuite:
- 12 cases for the native path: bounds combinations, duplicate values, nulls, DESC ordering, zero scale, high precision/scale, negative values crossing zero, fractional offset, and DECIMAL(38,0) max precision with values away from the limit.
- DECIMAL(38, 0) overflow: the [0, N, NULL] case above under both ANSI settings, which falls back and returns Spark's [2,2,1].
- DECIMAL(38, 0) at the limit: the largest safe offset over the type's extremes, 0 and a NULL, across ASC/DESC, NULLS FIRST/LAST and both ANSI settings, which runs natively and matches Spark.
- DECIMAL(38, 10): an offset one unscaled unit over the limit falls back and matches Spark, and the limit itself runs natively and matches Spark, across the same orderings and both ANSI settings.

How are these changes tested?

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

Both suites pass locally. With the guard removed, the DECIMAL(38, 0) overflow test returns [2,3,1] and the DECIMAL(38, 10) overflow test returns 2 instead of Spark's 1 for the largest value, so both pin the divergence. Spark negates a PRECEDING decimal offset with 34 significant digits, so the at-the-limit tests use the largest 34-digit offset under the limit for PRECEDING bounds.

…R BY

0lai0 · 2026-07-23T07:28:06Z

fixed ci test in window_function.sql

andygrove · 2026-07-24T14:54:53Z

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:

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.
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.

andygrove · 2026-08-27T15:57:14Z

Note on this review: this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled.

Good to see a fallback removed because the underlying problem was actually fixed upstream, and the rewritten comment in CometWindowExec explaining that WindowFrameTypeCoercion casts the DECIMAL offset while leaving the DATE offset as IntegerType is much more useful than the one it replaces. Turning the expect_fallback fixture into a plain query is the right signal that the guard is gone.

Two things.

The new "known difference" is a silent wrong answer under ANSI

when current +/- offset overflows the underlying integer, DataFusion collapses the bound to the partition edge, whereas Spark evaluates the boundary through normal decimal arithmetic (returning NULL under non-ANSI, throwing under ANSI)

Under ANSI, Spark throws and Comet returns a result. That is not a divergence in an edge-case value, it is a query that succeeds with wrong output where Spark would have failed. Comet's usual bar for that is to fall back rather than accept it.

Could the guard be narrowed rather than removed? Falling back only when the ORDER BY decimal is at or near DECIMAL_MAX_PRECISION would keep native execution for every realistic decimal while preserving Spark's behavior for the overflow case. That seems much better than trading a whole class of queries becoming native for a new silent-divergence bullet.

If you would rather accept the divergence, it needs a tracking issue linked from the docs bullet. Right now that paragraph is the only record that the difference exists, and there is nowhere for an affected user to go.

Confirm the upstream fix is actually in the pinned DataFusion

The description cites apache/datafusion#22174 as fixing the precision mismatch. Which DataFusion version does Comet pin today, and does it contain that PR? If Comet is still on a version without it, removing the guard would surface the original "Uncomparable values" error rather than a wrong answer. A line in the description confirming the pinned version includes the fix would settle it, and the 255 new lines in CometWindowExecSuite presumably demonstrate it, but it is worth stating.

sunchao

Summary

Prior state and problem: Explicit-offset RANGE windows ordered by DECIMAL fell back to Spark because DataFusion previously rejected comparisons between different decimal precisions.
Design approach: Remove that fallback and reuse existing native boundary arithmetic. DataFusion 54.1.0, pinned at this head, contains the required comparator fix.
Correctness / compatibility analysis: Checked Spark sources for 3.4.3, 3.5.8, 4.0.2, 4.1.2, and 4.2.0. All use DecimalAddNoOverflowCheck for these boundaries. Contrary to the existing discussion, boundary overflow does not generally produce NULL or an ANSI exception. However, the existing overflow concern remains substantiated: native overflow handling can incorrectly include null sort keys.
Key design decisions: Reusing the existing planner keeps the change small and introduces no new abstraction or per-row processing layer. No reproducible performance regression was established, and no benchmark was run.
Implementation sketch: Reviewed all five changed files, including the fallback removal, twelve Scala tests, SQL fixture, and documentation updates.
Behavioral changes worth calling out: With DECIMAL(38,0) values [0, N, NULL], where N = 90000000000000000000000000000000000000, ORDER BY d ASC NULLS LAST RANGE BETWEEN N PRECEDING AND N FOLLOWING produces COUNT results [2,2,1] in Spark under both ANSI settings. The pinned native frame implementation produces [2,3,1]: overflow moves the upper boundary past the trailing NULL. This substantiates the unresolved existing overflow concern, so it is not duplicated as a new finding.
Suggested improvements: Resolve that existing blocker by preserving Spark's null exclusion during overflow, or retaining fallback for affected frames. Add a regression covering overflow together with null ordering in both ANSI modes. No additional introduced P1/P2 issues found within this review.

Reviewed full SHA aca4d3c8dfa1c36f8041c3a581a4f7c9b3db1d25 against d075ab326e7601ffb65dc3d156ee4b00362ec06b. Live metadata confirmed non-draft status. Routed skills: review-comet-pr and review-comet-expression-pr. This checkout uses .claude/skills; the newer .ai guidance was read from another local Comet checkout.

Exact-head CI: zero check runs and zero workflow runs reported. Commit status is pending with no individual statuses.

Validation: Ran eight isolated native frame cases using DataFusion 54.1.0 and Arrow 58.3.0, with versions and checksums matching the head lockfile. Ran corresponding Spark 4.1.3 queries under both ANSI settings. Comet end-to-end tests could not run: the wrapper encountered read-only ~/.m2, and a writable Maven repository lacked dependencies while Maven Central access was blocked. Project code remains unchanged.

dwsmith1983 · 2026-10-04T04:40:22Z

This has been waiting since the 2026-07-24 and 2026-08-27 reviews asking to keep the overflow case on Spark by default. 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: Explicit-offset RANGE windows ordered by DECIMAL fell back to Spark because DataFusion previously rejected boundary comparisons across decimal precisions.
Design approach: Remove the decimal fallback and reuse existing native frame arithmetic. The pinned DataFusion 54.1.0 contains the comparator fix.
Correctness / compatibility analysis: Checked Spark sources for 3.4.3, 3.5.8, 4.0.2, 4.1.2, and 4.2.0. These boundaries use DecimalAddNoOverflowCheck, so the documentation’s claim that overflow returns NULL or throws under ANSI is inaccurate. The existing overflow/null-ordering blocker remains reproducible.
Key design decisions: Reusing the existing planner keeps the implementation small, with no new abstraction or processing layer. No reproducible performance regression was established. No benchmark was run.
Implementation sketch: Reviewed the entire five-file base-relative diff, including fallback removal, twelve Scala tests, the SQL fixture, and documentation changes.
Behavioral changes worth calling out: For DECIMAL(38,0) values [0, N, NULL], with N = 90000000000000000000000000000000000000, COUNT(*) OVER (ORDER BY d ASC NULLS LAST RANGE BETWEEN N PRECEDING AND N FOLLOWING) returns [2,2,1] in Spark under both ANSI settings. The pinned native frame implementation produces [2,3,1] because overflow includes the trailing null. This confirms the unresolved existing concern, already substantiated by the prior review, and is not duplicated as a new finding.
Suggested improvements: Resolve that blocker by preserving Spark’s null exclusion during boundary overflow or retaining fallback for affected frames. Add the demonstrated case as a regression test. No additional introduced P1/P2 issues found within this review.

Reviewed full SHA aca4d3c8dfa1c36f8041c3a581a4f7c9b3db1d25 against d075ab326e7601ffb65dc3d156ee4b00362ec06b. Live metadata confirmed non-draft status. Routed skills: review-comet-pr and review-comet-expression-pr. Read the pinned .claude guidance and the requested .ai skills from another local Comet checkout.

Exact-head CI: zero check runs and zero workflow runs. Commit status is pending with no individual statuses.

Validation: Reran eight isolated native frame cases against DataFusion 54.1.0 and Arrow 58.3.0, verifying relevant sources and crate checksums against the head lockfile. Ran all eight corresponding Spark 3.5.9 cases under both ANSI settings. Comet end-to-end tests could not run: the JVM build prerequisite failed on an uncached parent POM, and Maven Central access was blocked by network policy. Project code remains unchanged.

kazuyukitanimura · 2026-10-15T19:16:38Z

Hi @0lai0 Would you mind resolving the conflicts please?

# Conflicts: # docs/source/contributor-guide/roadmap.md # docs/source/user-guide/latest/compatibility/operators.md

# Conflicts: # docs/source/user-guide/latest/compatibility/operators.md

andygrove

Thanks @0lai0. Removing a fallback because the upstream problem was actually fixed is the right way to do this, and the twelve checkSparkAnswerAndOperator cases cover the normal ranges well. I compared against branch-1.1, which still falls back for DECIMAL RANGE offsets, so this PR moves that shape from Spark to native by default. For ordinary values that looks intended. For values near the Decimal128 limit I think it changes the answer, and I'm requesting changes for that reason.

With DECIMAL(38,0) values [0, N, NULL] and N = 90000000000000000000000000000000000000, the query ORDER BY d ASC NULLS LAST RANGE BETWEEN N PRECEDING AND N FOLLOWING returns counts [2,2,1] in Spark under both ANSI settings. The earlier review on this PR reproduced the native frame returning [2,3,1], because the overflowed upper bound moves past the trailing NULL. That is a wrong answer on a path that is correct on the release today. The "Known differences" entry documents it, but a documented divergence on a default-on path still needs to fall back. Could the guard in CometWindowExec fall back only when the ORDER BY decimal is at or near DECIMAL_MAX_PRECISION? That keeps every realistic decimal native and keeps Spark's result for the overflow shape. If you would rather fix it natively, the bound handling would need to keep excluding NULL keys on overflow.

Two related things.

On the docs, the new bullet says Spark returns NULL under non-ANSI and throws under ANSI when the boundary overflows. The earlier review checked Spark 3.4.3 through 4.2.0 and found that these boundaries use DecimalAddNoOverflowCheck, so neither happens. Could you recheck that against the Spark source and correct the text? If the guard is narrowed, this can become an entry in the fallback list above it instead.

On the tests, the DECIMAL(38, 0) case deliberately stays away from the overflow edge, so nothing pins the behavior that matters. Would you add a case with overflow together with NULLS LAST, run with spark.sql.ansi.enabled set to both true and false?

Smaller things that also need a pass:

The test header comment says DataFusion 54.0.0 is "already pinned", but main is on 55.2.0 now. Could it name the version that introduced the fix without claiming a pin?
The fixture comment in window_functions.sql says the DATE case "still falls back". #4974 removes that fallback, so whichever of the two PRs merges second should update the comment.

CI has not run on this branch yet. Since this touches the window serde and an SQL fixture, it needs a green run before it can go in.

# Conflicts: # docs/source/contributor-guide/roadmap.md # docs/source/user-guide/latest/compatibility/operators.md # spark/src/main/scala/org/apache/spark/sql/comet/CometWindowExec.scala # spark/src/test/scala/org/apache/comet/exec/CometWindowExecSuite.scala

0lai0 · 2026-10-17T13:01:26Z

Thanks for the careful review. Addressed in the latest commit:

Overflow: CometWindowExec now falls back only when the bound can overflow Decimal128, i.e. when the largest value of the ORDER BY type plus the offset exceeds 2^127 - 1 unscaled. DataFusion rescales both operands to the larger scale and adds them as i128, so DECIMAL(p, s) with p <= 37 can never overflow and stays native; only DECIMAL(38, s) with an offset above ~7.01e37 falls back.
Docs: you're right. WindowEvaluatorFactoryBase builds the bound with DecimalAddNoOverflowCheck, so Spark neither returns NULL nor throws. I removed the "Known differences" entry and replaced it with a fallback-list entry describing the actual divergence (the overflowing native bound becomes the partition edge and takes in the NULL keys there).
Tests: added your [0, N, NULL] / NULLS LAST case under both ANSI settings; it falls back and returns [2,2,1]. With the guard removed it reproduces [2,3,1]. Also added a case at the largest safe offset over the type's extremes and a NULL, across ASC/DESC and NULLS FIRST/LAST under both ANSI settings, which stays native and matches Spark. One note: Spark negates a PRECEDING decimal offset with 34 significant digits, so the test uses the largest 34-digit offset under the limit.
Fixed the DataFusion 54.0.0 wording and the stale DATE comment in window_functions.sql (apache/datafusion-comet#4974 has merged).

While checking this I found that the DATE threshold from apache/datafusion-comet#4974 has the same NULL-key issue once arrow's chrono range overflows. I'll fix that in a separate PR under apache/datafusion-comet#5022.

andygrove

Thanks @0lai0. The new guard fixes the problem I raised in my earlier review. I read window_state.rs in DataFusion 55.2.0 and add_decimal_values in ScalarValue. The bound is added as an i128 after rescaling both operands to the larger scale. Neither ScalarValue nor Arrow's with_precision_and_scale validates the result against the precision. So "the type's maximum plus the offset fits in an i128" is the right condition, and isDecimalRangeOffsetSafe computes exactly that. The [0, N, NULL] case with NULLS LAST now asserts both the fallback reason and Spark's [2,2,1] under both ANSI settings. The docs bullet also describes what actually happens now.

Against branch-1.1, which still falls back for every DECIMAL RANGE offset, the shapes that move to native are the ones that cannot overflow Decimal128. I don't see an unintended change there.

I have not run the suites, and CI has not run on this head yet. Since this touches the window serde and an SQL fixture, it needs a green run on 45b179fb before it can go in.

Two things to address before merge.

The comment on the DECIMAL(38, 0) max-precision test still says the values avoid the divergence between Spark ANSI and DataFusion's add_checked. Spark's DecimalAddNoOverflowCheck never throws, and the overflow case is now handled by the guard. Could the comment say that the values stay off the edge so the test exercises the native path?

Both new overflow tests use scale 0, so the 10^(scale - orderType.scale) and 10^(scale - d.scale) factors in isDecimalRangeOffsetSafe are never exercised. A mistake there would send an overflowing frame down the native path without any test noticing. Could you add a case with a non-zero scale, such as DECIMAL(38, 10), with an offset just over the limit that falls back and one just under it that stays native?

One more question. The docs bullet and the code comment link to #5022 for the DECIMAL case, but the issue body still says Spark returns NULL or throws under ANSI. That contradicts the Spark source and your docs change. Could you update the issue body, and narrow it to DATE now that DECIMAL falls back for the overflowing offsets?

Dismissing: the overflow guard in 45b179f addresses the divergence this review raised. Remaining asks are in the follow-up review.

0lai0 · 2026-10-18T16:20:42Z

Thanks @andygrove. Addressed in the latest commit:

Max-precision test comment: it now says the values and offset stay away from the Decimal128 limit so the test exercises the native path, and that the overflow case falls back and is covered by the tests below it.
Non-zero scale: added two DECIMAL(38, 10) tests over the type's extremes, 0 and a NULL, under both ANSI settings. An offset one unscaled unit over the limit (7014118346046923173168730371.5884105729) falls back and matches Spark; with the guard removed it returns 2 instead of Spark's 1 for the largest value, because the overflowing bound takes the trailing NULL in. The limit itself (7014118346046923173168730371.5884105728 FOLLOWING), and the largest 34-digit offset under it on both sides, run natively and match Spark across ASC/DESC and NULLS FIRST/LAST.

One note on the scale factors: Spark's WindowFrameCoercion (createBoundaryCast in TypeCoercionHelper) casts a numeric offset to the ORDER BY type, so by the time the guard sees it the two scales are equal and both factors are 10^0. The new tests do cover the non-zero scale in the type's largest unscaled value and in the offset's unscaled value. I kept the factors so the guard stays correct if an offset ever reaches it with a different scale.
RANGE window frame boundary overflow diverges from Spark for DATE and DECIMAL ORDER BY #5022: I don't have permission to edit the issue body, so I left a comment there with the corrected Spark behavior and the current state of DATE and DECIMAL, and asked you to update the description.

sunchao

Summary

Prior state and problem: Explicit-offset RANGE windows ordered by DECIMAL fell back to Spark because DataFusion previously rejected comparisons across decimal precisions.
Design approach: Replace the blanket fallback with isDecimalRangeOffsetSafe, retaining Spark execution when boundary arithmetic could overflow i128.
Correctness: The overflow calculation matches the pinned native arithmetic and addresses the previously reported overflow/null-key blocker. One new P2 remains: descending high-precision decimal offsets can produce different frame membership because Spark rounds during offset negation while native subtraction remains exact.
Compatibility analysis: Read the relevant Spark sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. All use DecimalAddNoOverflowCheck and negate offsets for descending order. Runtime checks reproduced the new mismatch on Spark 4.1.3, but not 3.5.9, demonstrating why the Scala-version-dependent decimal behavior matters. Compared with branch-1.1 at 89c729a9697467e8f8d89e3eadf1797a75e2328c, which retains the blanket decimal fallback.
Key design decisions: Checking both bounds conservatively at planning time avoids data-dependent overflow detection. Reusing DataFusion's existing comparator and frame implementation keeps the change small, but overflow safety alone does not establish Spark compatibility.
Implementation sketch: Reviewed the entire five-file base-relative diff, including the guard, sixteen added Scala tests, SQL fixture change, roadmap update, and compatibility documentation.
Performance: The added work evaluates at most two constant bounds and performs bounded BigInt arithmetic during planning. No per-row processing layer is introduced. No benchmark was run, and no reproducible performance regression was established.
Design: Narrowing the fallback is a reasonable design. It also needs to account for Spark's effective descending offset before declaring a frame compatible.
Abstraction & complexity: A private predicate beside the existing DATE guard is appropriately scoped. No new framework is needed. The compatibility decision must incorporate ordering-dependent negation semantics.
Behavioral changes worth calling out: Ordinary decimal-offset windows become native by default. Overflow-prone frames retain fallback. However, a safe DECIMAL(35,0) descending frame changes counts from Spark's [1,1] to native [2,1] in the reproduced case.
Suggested improvements: Preserve Spark's effective descending decimal offset, or retain fallback when negation changes its magnitude. Add the demonstrated boundary-membership regression across the relevant Spark profiles.

Reviewed full SHA 83a6bf3386685942c4a815ceee6f8c5885d605c3 against a0cbb0fa06515acd5647538025a3a894be71b9bd. Live metadata confirmed non-draft status. Routed skills: .ai/skills/review-comet-pr/SKILL.md and .ai/skills/review-comet-expression-pr/SKILL.md. Read existing reviews, conversation comments, and the empty inline-comment/thread collections. The earlier substantive overflow concern is addressed.

Exact-head CI: only the labeling check passed. Comet CI, CodeQL, and Check PR Title report action_required. There is no build/test verdict. Commit status is pending with no individual statuses.

Validation: Ran eighteen isolated native-frame cases with DataFusion 55.2.0 and Arrow 59.3.0, verifying relevant crate checksums and sources against the head lockfile. Compiled the exact-head overflow predicate in a disposable harness and checked five offsets. Ran Spark 4.1.3 and 3.5.9 reference queries under both ANSI settings. Full Comet suites and the Spark SQL matrix were not run. JVM test compilation stopped before compilation because the offline Maven cache lacked google-java-format:1.19.2. The finding is supported by component reproductions, not an end-to-end Comet run. Project source remains unchanged.

sunchao · 2026-10-19T15:22:45Z

            !ub.isInstanceOf[SpecialFrameBoundary] =>
        windowExpr.windowSpec.orderSpec.headOption.map(_.dataType) match {
-          case Some(_: DecimalType) =>
+          case Some(dt: DecimalType) if !Seq(lb, ub).forall(isDecimalRangeOffsetSafe(_, dt)) =>


[P2] [P2] Preserve Spark's descending decimal-offset negation

Could this fallback also account for offsets whose magnitude changes when Spark negates them for descending order? With d DECIMAL(35,0) containing [N,0], where N = 10000000000000000000000000000000001, COUNT(*) OVER (ORDER BY d DESC RANGE BETWEEN CURRENT ROW AND N FOLLOWING) returns [1,1] on Spark 4.1.3 under both ANSI settings. Spark's UnaryMinus(offset) rounds to -10000000000000000000000000000000000, leaving the first row's boundary at 1. This guard returns true, and rangeBoundLiteral preserves the positive offset exactly, so the native frame subtracts N, reaches 0, and returns [2,1]. The previous blanket fallback preserved Spark's result. Please retain fallback for affected descending offsets or serialize Spark's effective offset, with a regression containing rows at the differing boundary.

Evidence: Reproduction: create t with SELECT CAST(str AS DECIMAL(35,0)) AS d FROM VALUES ('10000000000000000000000000000000001'), ('0') AS v(str), then run SELECT d, COUNT(*) OVER (ORDER BY d DESC RANGE BETWEEN CURRENT ROW AND 10000000000000000000000000000000001 FOLLOWING) AS c FROM t ORDER BY d DESC. Spark 4.1.3 produced counts [1,1] with ANSI true and false. The compiled exact-head isDecimalRangeOffsetSafe returned true and the serialized d.abs retained all 35 digits. The lockfile-matching DataFusion 55.2.0/Arrow 59.3.0 WindowFrameContext, using CurrentRow and the exact decimal Following scalar, produced [2,1]. Also reproduced with DECIMAL(35,34) and offset 1.0000000000000000000000000000000001. Spark 3.5.9 returned [2,1], confirming the version-dependent behavior. Harnesses and logs are under /tmp/comet4987-83a6-review/.

Thanks @sunchao, good catch. Fixed in the latest commit.

For a descending ORDER BY, CometWindowExec now evaluates a DECIMAL frame offset as UnaryMinus(UnaryMinus(bound)): the same negation WindowEvaluatorFactoryBase applies, then negated back. On Scala 2.13 the first negation rounds to 34 significant digits exactly as Spark does, and negating the rounded value back is exact, so the native frame gets Spark's effective offset with the sign of the bound. On Scala 2.12 neither step rounds, which matches Spark 3.5 returning [2,1]. Both the serialized offset and the overflow guard use this value, since rounding can also push an offset up past the Decimal128 limit.

Added RANGE offset on DECIMAL ORDER BY uses the offset Spark evaluates, with your DECIMAL(35,0) case and the DECIMAL(35,34) one, over rows at the differing boundary (N, the smallest positive value, 0) and a NULL, across ASC/DESC, PRECEDING/FOLLOWING and both ANSI settings. It passes on the Spark 4.1 and 3.5 profiles.

This also changed one existing expectation: under a descending ORDER BY, the exact DECIMAL(38,10) limit rounds up past the limit when Spark negates it, so it now falls back, and the at-the-limit test uses the largest 34-digit offset under the limit for descending frames.

rich7420 · 2026-10-22T13:03:42Z

+-- Runs natively: Spark's WindowFrameTypeCoercion casts the frame offset to
+-- the same Decimal type as the ORDER BY column, so the boundary arithmetic
+-- keeps the current row's precision and DataFusion's comparator has matching
+-- operands. The DATE case in 1.11 keeps an IntegerType offset instead, which
+-- the native planner coerces to IntervalDayTime.


nit: Could we explain why this now passes? The bound still widens to DECIMAL(11,0); I think it works because DataFusion compares decimals with matching scales regardless of precision.

Suggested change

-- Runs natively: Spark's WindowFrameTypeCoercion casts the frame offset to

-- the same Decimal type as the ORDER BY column, so the boundary arithmetic

-- keeps the current row's precision and DataFusion's comparator has matching

-- operands. The DATE case in 1.11 keeps an IntegerType offset instead, which

-- the native planner coerces to IntervalDayTime.

-- Runs natively: Spark casts the frame offset to the ORDER BY decimal type.

-- Boundary arithmetic still widens precision (e.g. DECIMAL(11,0)), but

-- DataFusion compares Decimal128 values with matching scales regardless of

-- precision. The DATE case in 1.11 keeps an IntegerType offset instead, which

-- the native planner coerces to IntervalDayTime.

Agreed, applied your wording.

rich7420 · 2026-10-22T13:03:42Z

            !ub.isInstanceOf[SpecialFrameBoundary] =>
        windowExpr.windowSpec.orderSpec.headOption.map(_.dataType) match {
-          case Some(_: DecimalType) =>
+          case Some(dt: DecimalType) if !Seq(lb, ub).forall(isDecimalRangeOffsetSafe(_, dt)) =>


Could an ASC lower bound like -10000000000000000000000000000000001BD FOLLOWING lose precision here? d.abs in rangeBoundLiteral rounds beyond 34 digits in Scala 2.13. Maybe use Decimal(d.toJavaBigDecimal.abs, d.precision, d.scale) instead?

Yes, thanks. A negative literal like -10000000000000000000000000000000001 is parsed exactly (MINUS? INTEGER_VALUE), and an ascending ORDER BY adds it unchanged, but Decimal.abs negates it and rounds to 34 digits on Scala 2.13. rangeBoundLiteral now uses Decimal(d.toJavaBigDecimal.abs, d.precision, d.scale) as you suggested. The new test covers -N FOLLOWING AND CURRENT ROW and CURRENT ROW AND -N PRECEDING; with d.abs it fails (the N row counts 2 natively against Spark's 3), and with the exact absolute value it matches Spark.

…o pr-4987-merge # Conflicts: # docs/source/user-guide/latest/compatibility/operators.md # spark/src/main/scala/org/apache/spark/sql/comet/CometWindowExec.scala

…negative bounds

sunchao

I traced the descending decimal bound through Spark’s two negations and the rescaling guard. The ordinary frame cases can share the SQL fixture this PR already changes. I also found no completed CI run on this head.

sunchao · 2026-10-31T19:39:08Z

+      .parquet(dir.toString)
+  }
+
+  test("window: RANGE BETWEEN N PRECEDING AND N FOLLOWING with DECIMAL ORDER BY") {


Several tests rewrite the same Parquet input and vary only the SQL frame. Could the ordinary preceding/following cases share one table in windows/window_functions.sql? The runner compares rows and checks native execution. The tests that specifically distinguish CometWindowExec from a fallback at the precision boundary still have a reason to stay in Scala.

comphead

Thanks @0lai0 for working through the overflow and the descending rounding cases. I read the guard and evalRangeOffset against WindowEvaluatorFactoryBase and Decimal.unary_- at Spark v3.5.8 and v4.1.1, and against calculate_index_of_row in DataFusion 55.2.0. I did not find anything further on the semantics. I have not run the suites.

Two things outside the diff:

#4974 covered the DATE half of #4834, this PR covers the DECIMAL half, and the roadmap entry for #4834 is removed here. Should the description say Closes #4834 so the issue closes when this merges?
The descending offset now depends on the Scala version (2.13 rounds the negation, 2.12 does not). The PR tier runs the Comet suites only on Spark 4.1 with Scala 2.13, and the Spark 3.4 and 3.5 profiles, which build with Scala 2.12, run only nightly. Could a maintainer apply run-all-spark-profiles so the Scala 2.12 path of RANGE offset on DECIMAL ORDER BY uses the offset Spark evaluates runs before merge?

comphead · 2026-11-05T21:19:47Z

+      .parquet(dir.toString)
+  }
+
+  test("window: RANGE offset on DECIMAL(38, 0) ORDER BY that overflows Decimal128 falls back") {


Following up on @sunchao's note that the boundary tests have a reason to stay in Scala: I think they can move to a fixture too. query expect_fallback(<reason>) in CometSqlFileTestSuite calls checkSparkAnswerAndFallbackReason, the same check this test makes, and a file-level -- ConfigMatrix: spark.sql.ansi.enabled=true,false runs every query under both ANSI settings. Each ASC/DESC and NULLS FIRST/LAST combination becomes one query over a shared table, so a new file such as windows/window_range_decimal_offset.sql could hold these five tests and the ordinary frame cases above. The hardcoded checkAnswer of [2,2,1] and the CometWindowExec absence check would go, but the answer is still compared with Spark, and the reason is only recorded right before return None. I have not run it.

Thanks @comphead for checking the semantics against both Spark versions and DataFusion.

Closes Support RANGE window frames with an explicit offset on DATE or DECIMAL ORDER BY #4834: agreed, feat: native RANGE window frames with explicit offset on DATE ORDER BY #4974 covered DATE and this covers DECIMAL, so I've updated the description to Closes #4834. It still says Part of #5022, since the DATE side of the boundary overflow has a follow-up.

Scala 2.12: good point. Locally the DECIMAL tests in CometWindowExecSuite, including RANGE offset on DECIMAL ORDER BY uses the offset Spark evaluates, pass on the spark-3.5 profile (Scala 2.12) as well as Spark 4.1, but I haven't run Spark 3.4, and a local run is no substitute for CI. Could maintainer apply run-all-spark-profiles?

Moving the boundary tests to a fixture: shortly after your review I pushed 0ab79db, which moved the ordinary frame cases into section 1.12 of windows/window_functions.sql as @sunchao suggested. Only the five boundary tests are left in Scala. Your approach sounds right to me: expect_fallback makes the same check, and since the runner compares against whichever Spark is running, the Scala 2.12 and 2.13 expectations both stay correct. A file-level ConfigMatrix would run every query in window_functions.sql twice, so I'd move the ordinary cases out of it into a new windows/window_range_decimal_offset.sql together with the five boundary tests. @sunchao, since you suggested keeping the boundary tests in Scala, are you OK with this? If so I'll make the change.

Thanks both!!

….sql

0lai0 and others added 4 commits July 21, 2026 15:37

feat: native RANGE window frames with explicit offset on decimal ORDE…

42b64bf

…R BY

Merge branch 'main' into feat-4834-range-offset-decimal-order-by

de51bd4

Modified SQL test

2c19ce6

Merge branch 'main' into feat-4834-range-offset-decimal-order-by

aca4d3c

This was referenced Jul 24, 2026

RANGE window frame boundary overflow diverges from Spark for DATE and DECIMAL ORDER BY #5022

Open

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

Merged

andygrove added the enhancement New feature or request label Sep 6, 2026

sunchao reviewed Sep 25, 2026

View reviewed changes

sunchao reviewed Oct 4, 2026

View reviewed changes

0lai0 added 2 commits October 16, 2026 00:30

Merge remote-tracking branch 'upstream/main' into pr-4987-conflict

7468551

# Conflicts: # docs/source/contributor-guide/roadmap.md # docs/source/user-guide/latest/compatibility/operators.md

Merge remote-tracking branch 'upstream/main' into pr-4987-conflict

76f8ba2

# Conflicts: # docs/source/user-guide/latest/compatibility/operators.md

andygrove previously requested changes Oct 16, 2026

View reviewed changes

0lai0 added 2 commits October 17, 2026 15:56

fix: fall back for DECIMAL RANGE offsets that can overflow Decimal128

45b179f

andygrove reviewed Oct 17, 2026

View reviewed changes

test: cover a non-zero scale at the DECIMAL RANGE offset limit

83a6bf3

sunchao reviewed Oct 19, 2026

View reviewed changes

rich7420 reviewed Oct 22, 2026

View reviewed changes

0lai0 added 2 commits October 30, 2026 16:01

fix: use the DECIMAL RANGE offset Spark evaluates for descending and …

3e8fb02

…negative bounds

sunchao reviewed Oct 31, 2026

View reviewed changes

comphead reviewed Nov 5, 2026

View reviewed changes

test: move the ordinary DECIMAL RANGE frame cases to window_functions…

0ab79db

….sql

Conversation

0lai0 commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

0lai0 commented Jul 23, 2026

Uh oh!

andygrove commented Jul 24, 2026

Uh oh!

andygrove commented Aug 27, 2026

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

dwsmith1983 commented Oct 4, 2026

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

kazuyukitanimura commented Oct 15, 2026

Uh oh!

andygrove left a comment

Choose a reason for hiding this comment

Uh oh!

0lai0 commented Oct 17, 2026

Uh oh!

andygrove left a comment

Choose a reason for hiding this comment

Uh oh!

0lai0 commented Oct 18, 2026

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0lai0 Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

0lai0 commented Jul 21, 2026 •

edited

Loading

0lai0 Oct 30, 2026 •

edited

Loading