-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: generate_series overflow panics at i64 boundary and out-of-range dates #23723
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
u70b3
wants to merge
4
commits into
apache:main
Choose a base branch
from
u70b3:fix/generate-series-overflow
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+180
−5
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
742091b
fix: generate_series overflow panics at i64 boundary and out-of-range…
u70b3 c456e67
fix: stop generate_series at terminal boundary
u70b3 fc1c1fe
fix: preserve SeriesValue advance API
u70b3 f949de1
fix: stop timestamp series at range boundary
u70b3 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -197,6 +197,68 @@ SELECT * FROM generate_series(1, 2, 3, 4) | |
| statement error DataFusion error: Error during planning: Argument \#1 must be an INTEGER, TIMESTAMP, DATE or NULL, got Utf8 | ||
| SELECT * FROM generate_series('foo', 'bar') | ||
|
|
||
| # Regression test for https://github.com/apache/datafusion/issues/22208 | ||
| # A step that would overflow i64 after the last reachable value must return the | ||
| # reachable values instead of panicking, matching PostgreSQL/DuckDB behavior. | ||
| query I | ||
| SELECT * FROM generate_series(9223372036854775806, 9223372036854775807, 2) | ||
| ---- | ||
| 9223372036854775806 | ||
|
|
||
| # Same, in the descending direction | ||
| query I | ||
| SELECT * FROM generate_series(-9223372036854775806, -9223372036854775808, -2) | ||
| ---- | ||
| -9223372036854775806 | ||
| -9223372036854775808 | ||
|
|
||
| # Landing exactly on i64::MAX must include it | ||
| query I | ||
| SELECT * FROM generate_series(9223372036854775805, 9223372036854775807, 2) | ||
| ---- | ||
| 9223372036854775805 | ||
| 9223372036854775807 | ||
|
|
||
| # Same overflow behavior for `range` (end exclusive) | ||
| query I | ||
| SELECT * FROM range(9223372036854775806, 9223372036854775807, 2) | ||
| ---- | ||
| 9223372036854775806 | ||
|
|
||
| # Regression test for https://github.com/apache/datafusion/issues/22193 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be helpful to add positive boundary tests for the maximum representable date and timestamp, alongside the existing out-of-range error cases. That would lock in the expected behaviour of returning all reachable values at the edge of the nanosecond range. |
||
| # Dates outside the nanosecond timestamp range must produce a clean planning | ||
| # error instead of panicking (debug) or silently wrapping (release). | ||
| statement error DataFusion error: Error during planning: First argument for generate_series is out of range of nanosecond timestamps | ||
| SELECT * FROM generate_series(DATE '0001-01-01', DATE '2000-01-01', INTERVAL '1' DAY) | ||
|
|
||
| statement error DataFusion error: Error during planning: Second argument for generate_series is out of range of nanosecond timestamps | ||
| SELECT * FROM generate_series(DATE '2000-01-01', DATE '3000-01-01', INTERVAL '1' DAY) | ||
|
|
||
| # Reaching the maximum representable date must not attempt to advance beyond it. | ||
| query P | ||
| SELECT * FROM generate_series(DATE '2262-04-11', DATE '2262-04-11', INTERVAL '1' DAY) | ||
| ---- | ||
| 2262-04-11T00:00:00 | ||
|
|
||
| # Same for the maximum representable nanosecond timestamp. | ||
| query P | ||
| SELECT * FROM generate_series(TIMESTAMP '2262-04-11T23:47:16.854775807', TIMESTAMP '2262-04-11T23:47:16.854775807', INTERVAL '1' NANOSECOND) | ||
| ---- | ||
| 2262-04-11T23:47:16.854775807 | ||
|
|
||
| # A timestamp step that exceeds the nanosecond range must terminate after the | ||
| # last reachable value instead of returning an overflow error. | ||
| query P | ||
| SELECT * FROM generate_series(TIMESTAMP '2262-04-11T23:47:16.854775806', TIMESTAMP '2262-04-11T23:47:16.854775807', INTERVAL '2' NANOSECOND) | ||
| ---- | ||
| 2262-04-11T23:47:16.854775806 | ||
|
|
||
| # Same behavior for date series, which use the timestamp implementation. | ||
| query P | ||
| SELECT * FROM generate_series(DATE '2262-04-10', DATE '2262-04-11', INTERVAL '2' DAY) | ||
| ---- | ||
| 2262-04-10T00:00:00 | ||
|
|
||
| # UDF and UDTF `generate_series` can be used simultaneously | ||
| query ? rowsort | ||
| SELECT generate_series(1, t1.end) FROM generate_series(3, 5) as t1(end) | ||
|
|
||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new
finishedguard fixes the exact boundary case by avoiding an extraadvanceonce the current value is already the final emitted value. I think there is still one edge case left, though.Consider:
Here the current value is the last reachable value before
end, but line 453 still callsadvance, which attempts to compute...775808and returns the Arrow overflow error.I'd expect this to behave like the integer overflow case and return the single reachable row (
...775806) without an error.Could we apply the same overflow-aware terminal handling to timestamp and date series as well? It would also be great to add an SLT covering a near-maximum timestamp or date where the next step would overshoot the representable range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kosiew — addressed in b4fcb2e.
TimestampValuenow overridesadvance_with_end, treating an unrepresentable next timestamp as terminal using the same end-clamping approach asi64; this also covers date series because they useTimestampValue. I added SLT cases for a near-maximum timestamp and date where the next step exceeds the nanosecond range.