Repository navigation
Conversation
|
fixed ci test in window_function.sql |
|
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 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 Two ways to resolve it, in order of preference:
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:
|
Good to see a fallback removed because the underlying problem was actually fixed upstream, and the rewritten comment in Two things. The new "known difference" is a silent wrong answer 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 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 |
sunchao
left a comment
There was a problem hiding this comment.
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, and4.2.0. All useDecimalAddNoOverflowCheckfor 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], whereN = 90000000000000000000000000000000000000,ORDER BY d ASC NULLS LAST RANGE BETWEEN N PRECEDING AND N FOLLOWINGproduces 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.
|
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
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Explicit-offset
RANGEwindows ordered byDECIMALfell 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.0contains the comparator fix. - Correctness / compatibility analysis: Checked Spark sources for
3.4.3,3.5.8,4.0.2,4.1.2, and4.2.0. These boundaries useDecimalAddNoOverflowCheck, 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], withN = 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.
|
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
left a comment
There was a problem hiding this comment.
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
mainis on 55.2.0 now. Could it name the version that introduced the fix without claiming a pin? - The fixture comment in
window_functions.sqlsays theDATEcase "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
|
Thanks for the careful review. Addressed in the latest commit:
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
left a comment
There was a problem hiding this comment.
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.
|
Thanks @andygrove. Addressed in the latest commit:
|
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Explicit-offset
RANGEwindows ordered byDECIMALfell 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, and4.2.0. All useDecimalAddNoOverflowCheckand negate offsets for descending order. Runtime checks reproduced the new mismatch on Spark4.1.3, but not3.5.9, demonstrating why the Scala-version-dependent decimal behavior matters. Compared withbranch-1.1at89c729a9697467e8f8d89e3eadf1797a75e2328c, 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
BigIntarithmetic 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.
| !ub.isInstanceOf[SpecialFrameBoundary] => | ||
| windowExpr.windowSpec.orderSpec.headOption.map(_.dataType) match { | ||
| case Some(_: DecimalType) => | ||
| case Some(dt: DecimalType) if !Seq(lb, ub).forall(isDecimalRangeOffsetSafe(_, dt)) => |
There was a problem hiding this comment.
[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/.
Which issue does this PR close?
Part of #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
RANGEframes with an explicitPRECEDING/FOLLOWINGoffset whenORDER BYwasDECIMAL. 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_cmpnow ignores precision forDecimal128when the scales match. The fix was first released in DataFusion 54.0.0, so DECIMALRANGEboundary 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
DECIMALbound withDecimalAddNoOverflowCheck(WindowEvaluatorFactoryBase), which keeps the exact value: it neither returnsNULLnor throws. DataFusion rescales both operands to the larger scale and adds them as ani128, and when that overflows it takes the partition edge as the bound, which also takes in theNULLkeys sorted at that edge. For example,DECIMAL(38,0)values[0, N, NULL]withN = 9e37andORDER BY d ASC NULLS LAST RANGE BETWEEN N PRECEDING AND N FOLLOWINGgive[2,2,1]in Spark and[2,3,1]natively. The bound can only overflow when the largest value of theORDER BYtype plus the offset exceeds2^127 - 1unscaled, which onlyDECIMAL(38, s)with an offset above about7.01e37unscaled reaches.What changes are included in this PR?
DecimalTypeRANGEfallback withisDecimalRangeOffsetSafe, which falls back only when the largest value of theORDER BYtype plus the offset can exceed2^127 - 1unscaled. EveryDECIMAL(p, s)withp <= 37, andDECIMAL(38, s)with any offset below that limit, now runs natively.roadmap.md, and replace theDECIMALRANGEentry in the fallback list inoperators.mdwith one for offsets whose bound can overflowDecimal128.DECIMAL ORDER BY DESCcase inwindow_functions.sqlnow runs natively, and its comment no longer says theDATEcase falls back (feat: native RANGE window frames with explicit offset on DATE ORDER BY #4974 removed that fallback).CometWindowExecSuite: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,0and aNULL, 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?
Both suites pass locally. With the guard removed, the
DECIMAL(38, 0)overflow test returns[2,3,1]and theDECIMAL(38, 10)overflow test returns2instead of Spark's1for the largest value, so both pin the divergence. Spark negates aPRECEDINGdecimal offset with 34 significant digits, so the at-the-limit tests use the largest 34-digit offset under the limit forPRECEDINGbounds.