feat: fix windows frame positive/neg overflows#22140
Merged
Merged
Conversation
waynexia
approved these changes
May 13, 2026
| Err(_) => return Ok(unbounded_edge), | ||
| } | ||
| if SEARCH_SIDE == is_descending { | ||
| // TODO: Handle positive overflows. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
RANGEwindow frames with a value offset (e.g.RANGE BETWEEN 4 PRECEDING AND 4 FOLLOWING) panicked withattempt to add/subtract with overflowwhenever the boundary target (value ± delta) wrappedpast the type's representable range. Affected inputs include values close to
i64::MAX/i64::MIN,u64::MAX, and any analogous boundary for other integer/decimal/timestamp types.Two
// TODO: Handle ... overflows.markers inWindowFrameStateRange::calculate_index_of_rowhad been left for this case; the uncheckedScalarValue::add/subsilently wrapped the target, afterwhich
search_in_slicewas handed a nonsensical (wrapped) value and downstream code tripped a debug-assert subtraction infunctions-window/src/nth_value.rs.Semantically, an overflowed boundary is unbounded with respect to the data in the partition — every real value lies strictly inside the wrapped sentinel — so the correct behavior is to collapse the
search to the appropriate partition edge rather than to search with a wrapped target.
What changes are included in this PR?
datafusion/expr/src/window_state.rsScalarValue::add/subwith their*_checkedcounterparts in the boundary computation.search_startforPRECEDING-direction searches,lengthforFOLLOWING-direction searches. The collapse direction depends only on theconst-generic
SEARCH_SIDE(the add branch and sub branch both reduce to!SEARCH_SIDEonce you expand theSEARCH_SIDE == is_descendinginvariant that selects each arithmetic branch).value.is_unsigned() && value < deltaclamp-to-zero path for unsigned subtraction is preserved — it produces a valid polymorphic zero, not an overflow sentinel.datafusion/sqllogictest/test_files/window.sltRegression coverage for positive and negative overflow, across:
ASC+FOLLOWING/ASC+PRECEDING/DESC+PRECEDING/DESC+FOLLOWING(each overflow direction occurs on both sort orders depending on which arithmetic branch is taken)N PRECEDING AND N FOLLOWINGframes where only one side overflowsi64) and unsigned (u64) ordering columnsfirst_valueandlast_valueboth exercised to verify both frame edgesROWSframe regression guard to document that the pre-existingsaturating_sub/min(length)saturation behavior is unchanged.