GeezerVison is showing github.com — [sqllib] Fix overflow bugs by mihaibudiu · Pull Request #7691 · feldera/feldera
Conversation
mihaibudiu
commented
Oct 28, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
Checklist
A code audit found misbehaviors in arithmetic around extreme values (e.g., abs(MIN_VALUE) was negative).
There are 4 commits, each with an explanatory commit message.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
What I ran
| Check |
Result |
cargo test --locked -p feldera-fxp |
✅ 43 passed |
cargo test --locked -p feldera-sqllib (abs/neg/round/interval filters) |
✅ |
mvn test FunctionsTest, PostgresInt8Tests, BasicQueriesTests#negate*, CastTests#castInterval, TimeTests#makeTime*, VariantTests#numberWith* |
✅ 67 passed |
Extra QA (scratch tests, not pushed): -TINYINT min and ABS(SMALLINT) min report overflow; -/ABS on nullable columns; negating the minimum INTERVAL MONTH |
✅ |
Extra QA: DynamicDecimal(-10^37) + 0.25 |
❌ multiply overflow (see inline) |
Findings
- bug (medium):
DynamicDecimal add with a large negative operand overflows in the fast path. It panics in debug builds and wraps silently in release builds, and it never reaches the fixed 256-bit branch. The bug predates this PR, but it is in the code being fixed, and add_wide only tests positive operands.
- missing (medium), commit quality:
[SQL] Do not warn when a Calcite optimization step fails has no body. It turns a user-visible compiler warning into a debug log, so the history should say why (is it the new non-foldable ABS making a reduce step throw?).
- missing (low-medium), tests: no SQL-level test for ABS of the minimum
TINYINT/SMALLINT/INT, or for -x with TINYINT/INT columns. I checked these by hand and they pass, but they aren't in the suite.
Otherwise the gates are met: tests cover each fix, docs are updated (integer.md), there are no new deps or unsafe blocks, and there is no flakiness risk.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…IMAL ROUND
- DynamicDecimal multiplication and the 256-bit branches of addition added
the digits dropped from the result to the exponent instead of subtracting
them. CAST(DECIMAL AS INTERVAL DAYS) and MAKE_TIME with many fractional
digits gave small wrong values.
- The conversion of a DynamicDecimal with more than 38 fractional digits to
DOUBLE panicked, e.g. CAST(PARSE_JSON('1e-100') AS DOUBLE).
- LongInterval::years negated the months, which overflows for the minimum
interval.
- ROUND and BROUND of a DECIMAL panicked when rounding more than 38 digits
away from the last digit of the scale.
Signed-off-by: Mihai Budiu <[email protected]>
- ABS of the minimum integer or interval, and unary minus of the minimum
integer now report an overflow.
Signed-off-by: Mihai Budiu <[email protected]>
…gits
10^digits is infinity or 0 for a large digits argument: ROUND(1.5e0, 400)
was NaN, and ROUND(1e300, 20) was infinity.
Signed-off-by: Mihai Budiu <[email protected]>
Adding two DynamicDecimal values whose exponents differed by more than
38 panicked, because the value with fewer fractional digits was scaled
by a power of 10 that does not fit in an i128. The datagen connector
adds such values for a DECIMAL range bound with more than 38 fractional
digits. The sum now keeps the digits of the other value down to 37
places past the last digit of the first, and is truncated toward zero.
Signed-off-by: Mihai Budiu <[email protected]>
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
blp
blp approved these changes
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|