fix: fall back for ANSI and TRY integer sums over sliding windows

[rich7420]

Which issue does this PR close?

Closes #6043.

Rationale for this change

Sliding integer sums wrap on overflow, violating ANSI sum and try_sum semantics.

What changes are included in this PR?

Fall back to Spark for ANSI/TRY integral sums over sliding frames. Legacy sums and ever-expanding frames retain native execution. Update compatibility notes and window test expectations.

How are these changes tested?

SQL regressions cover overflow, NULL/empty frames, recovery, ROWS/RANGE bounds, and native controls. New and affected tests passed on Spark 3.5.9 and 4.1.3.

Fork CI passed: default checks, Comet Spark 3.4–4.2 tests, and Spark 4.1 SQL tests.

[sunchao]

Correctness

Reviewed 46ef424d0261f73ea943af7160ca410c77c9854b against 6065705c16340c0be293212a71decfd9df4daae4. No verified P1/P2 findings.

The prior sliding path uses DataFusion's integer sum accumulator, which wraps on overflow without honoring the aggregate's ANSI/TRY mode. For a one-row-preceding frame over [Long.MaxValue, 1, -1], Spark's try_sum returns [Long.MaxValue, NULL, 0]; the wrapping path produces Long.MinValue at the second row. Spark recomputes changing frames, so the result can recover after the overflowing values leave.

The new admission guard correctly combines a lower bound other than UNBOUNDED PRECEDING, a LongType sum result, and a non-legacy aggregate evaluation mode. Spark promotes byte, short, int and long sums to long. Reading the mode through the existing shim also catches try_sum when session ANSI is disabled. Returning None follows the existing whole-WindowExec fallback contract, including shrinking, singleton and empty frames. Legacy integral sums, floating-point sums, and expanding integral sums retain their existing native paths; expanding sums use Comet's mode-aware accumulator.

Validation

• Compared Spark's sum state and moving/growing/shrinking frame evaluation on the maintained 3.5 and 4.0 branches. Checked all four Comet version shims and the checksum-verified DataFusion 55.1.0 sliding accumulator.
• The current expressions CI job actually ran windows/sliding_integer_sum.sql. Its nine fallback queries check the expected reason and Spark-equivalent answers; five ordinary queries assert native coverage; three error queries compare overflow failures. Coverage includes both overflow directions, recovery, null/empty frames, ROWS/RANGE, narrow integral inputs and native controls.
• The current execution CI job passed the affected window tests. Both jobs checked out merge 908f9931454e, whose tree equals the reviewed head, and consumed the native artifact built by the same run with matching upload/download SHA-256 digests.
• Local validation comprised source assertions and an independent signed-64 arithmetic model. The required fallback reason is absent from the base implementation, establishing the fixture's pre-fix failure at the source-contract level; I did not run the pre-fix Spark/JNI implementation. Current runtime evidence is Spark 4.1 CI. Canonical maintained 3.4 and 4.1 source branches were unavailable, so source-semantic coverage is limited to 3.5/4.0. The expressions job's one canceled test is an unrelated Spark-4.0-only regexp reproducer.

Performance

The extra work is a small planning-time type/mode check, with no per-row allocation or arithmetic added. Affected windows now execute in Spark, and one rejected expression causes the whole window operator to fall back. That can increase query time and JVM buffering/recomputation costs, including the quadratic shrinking-frame case, but it is the explicit cost of preventing incorrect overflow results. The guard is independent of input values and therefore also applies when a particular dataset would not overflow. Native legacy and expanding controls keep the unaffected cases covered. No performance measurement or speedup claim is attached to this change.

Design

The fix fits the existing sliding-decimal fallback and uses the same frame classification as native accumulator selection. Keeping the decision in window admission avoids changing grouped sums or pretending that a sticky overflow flag can support retraction. A native alternative would need to reproduce Spark's overflow and recovery semantics as frame contents change, which is a larger change than this compatibility guard. The fallback reason and compatibility documentation explain the resulting boundary.

Abstraction & complexity

The implementation reuses the existing evaluation-mode shim and fallback machinery. It introduces no new runtime abstraction or ownership path. The small test helper adapts existing plain-sum assertions to the session's ANSI mode, while the SQL fixture separately tests explicit TRY mode with ANSI both disabled and enabled. The focused production change, regression fixture and documentation form a coherent scope; I found no actionable simplification.

[andygrove]

Thanks for this. I traced the premise through the native side and it checks out. process_agg_func in native/core/src/execution/planner.rs only builds the mode-aware SumInteger UDAF when is_ever_expanding is true, and sliding frames fall through to DataFusion's built-in sum, which wraps and has no notion of eval mode. So the divergence was real, and the admission guard is at the right layer, matching the decimal guard from #4732.

A few things I checked that look fine, noted here so they do not get re-litigated. s.dataType == LongType is the right predicate, because Spark promotes byte, short, int and long sums to LongType, and interval sums are already rejected earlier by AggSerde.sumDataTypeSupported. Shim coverage is not a concern either, since sumEvalMode already exists across the 3.4, 3.5, 4.0 and 4.1+ shims. I also suspected a matching gap in integral avg, but Spark's Average accumulates integral input in DoubleType, so there is no ANSI overflow there and nothing to fix.

The fixture is thorough on the behaviour it covers, including both overflow directions, recovery, null and empty frames, and native controls on the paths that stay native. Both of my comments are about the test harness rather than the fix itself.

[andygrove]

Thanks for the comment explaining why this is here, it made the intent easy to follow. One concern though. CometSqlFileTestSuite excludes ConstantFolding on purpose, and appends that exclusion after each file's own configs so that a header cannot override it. Clearing it with a SET statement turns folding back on for every query in the rest of the file, including the plain query entries that assert native coverage. This is the only fixture in the repo that does this.

window_functions.sql lines 198 to 203 hit the same ROWS ... N PRECEDING problem and took the other route, keeping SQL fixtures to bounds that parse directly and covering N PRECEDING in CometWindowExecSuite through the DataFrame API. Could we follow that convention here? The RANGE ... 1 PRECEDING cases already work unfolded, as window_functions.sql line 226 shows, so only the ROWS ones would need to move and the SET could go away entirely.

If you think the fixture really does need folding, could we add a supported file-level directive for it instead, so the harness stays in control of what it guarantees?

[andygrove]

I follow why this is needed. The default profile is Spark 4.1.3 where ANSI is on, so without this the sliding sums in this matrix would now fall back and checkSparkAnswerAndOperator would fail its native coverage assertion.

The side effect is that this matrix stops exercising ANSI at all, and it covers a good deal more than sums. Would routing the sum assertions through checkSlidingIntegralSum work here, the way the three tests below do, so the matrix keeps running in both modes? If pinning really is the cleaner option, could the comment mention that the matrix no longer covers ANSI? As written it reads as being only about legacy sums, and the next person may not realise the broader coverage went with it.

[andygrove]

Is spark.comet.operator.WindowExec.allowIncompatible=false doing anything here? CometWindowExec does not declare a support level, and the only other fixture using that key, lag_lead.sql, sets it to true. If this is just documenting the default, it may be worth dropping so that it does not get copied into future fixtures as boilerplate.

[sunchao]

Rechecked the unchanged 46ef424d after the new harness feedback. I agree with the existing comments: the fixture-wide SET clears the harness’s folding exclusion, the window matrix now forces legacy mode for non-sum cases too, and WindowExec.allowIncompatible has no effect on this conversion guard. Those threads cover the needed test changes. I found no additional P1/P2 to raise.

One coverage qualification: the matrix previously inherited the session’s ANSI mode. It was not an explicit two-mode matrix. The new fixture still tests ANSI/TRY fallback and errors, and its table-column sums are not folded away. Its five ordinary queries retain native-plan assertions.

Fresh log checks confirm that the SQL fixture and window suite passed on CI merge 908f9931, whose parents are the reviewed base/head and whose tree equals this head. This follow-up used source checks, with no new local Spark/JNI run. Maintained Spark 3.5/4.0 semantics were rechecked; the 3.4/4.1 source gaps remain. My existing approval is preserved.