GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — [fxp] Round half-even values below one unit to ±1, not 0. by blp · Pull Request #7796 · feldera/feldera

Conversation

blp commented Nov 3, 2026

Copy link

Copy Markdown

Member

i128_mul_pow10_round_even() (crates/fxp/src/lib.rs) stepped away from zero by the sign of the truncated quotient. When the quotient was 0, the step was 0, so values such as 0.6 at scale 0 rounded to 0 instead of 1. This change steps by the sign of the value instead, as round_inner() already does.

This affects every half-even path in fxp: Fixed::from_str, convert_round_even, new_round_even and DynamicDecimal::into_fixed_round_even. In SQL, it affects CAST('0.6' AS DECIMAL(5, 0)) and DECIMAL to TIMESTAMP casts.

The new test round_even_below_one fails without the fix and passes with it.

Fixes #7717.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mythical-fred-oss Bot commented Nov 3, 2026

Copy link

Copy Markdown

Contributor

LGTM. The one-line fix makes i128_mul_pow10_round_even() step by value.signum(), the same way round_inner() and sqllib's div_round_nearest() already do. Overflow is not possible because divisor >= 10.

What I ran

Check	Result
`cargo test --locked -p feldera-fxp`	✅ 47 + doc tests pass
`round_even_below_one` with the old `quotient.signum()`	❌ fails at line 1807, so the test does catch the bug
Extra QA cases (scratch, not pushed): `-1.5→-2`, `-2.5→-2`, `±0.5000001→±1`, exponent -38 (`±0.6e38/1e38→±1`), `i128::MAX` at exponent -39 `→0`, and `DynamicDecimal::into_fixed_round_even` for `±0.0006→±0.001`	✅ all pass
`cargo fmt --check`	✅

Missing (medium): no SQL-level regression test. The description says this changes CAST('0.6' AS DECIMAL(5, 0)) and DECIMAL→TIMESTAMP casts, but nothing in the SQL compiler or Python runtime tests covers either case. A short end-to-end test with 0.6, -0.6, 0.5 and a sub-millisecond DECIMAL→TIMESTAMP cast would keep the SQL behaviour from regressing if the compiler later routes these casts through a different code path.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mihaibudiu approved these changes

Nov 3, 2026

View reviewed changes

mihaibudiu left a comment

Copy link

Copy Markdown

Contributor

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.

Does this affect gen2?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

blp self-assigned this

Nov 3, 2026

blp added bug

Something isn't working

Support Bundle

Things that concerns support bundles collections

rust Pull requests that update Rust code and removed Support Bundle Things that concerns support bundles collections labels

Nov 3, 2026

blp commented Nov 3, 2026

Copy link

Copy Markdown

Member Author

Does this affect gen2?

I assume so, since I imagine it uses fxp

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          [fxp] Round half-even values below one unit to ±1, not 0.

7f9fee6

i128_mul_pow10_round_even() stepped away from zero by the sign of the
truncated quotient.  When the quotient was 0, the step was 0, so values
such as 0.6 at scale 0 rounded to 0 instead of 1.  Step by the sign of
the value instead, as round_inner() does.

Add tests in fxp and in the SQL compiler, for CAST of VARCHAR to DECIMAL
and of DECIMAL to TIMESTAMP.

Fixes #7717.

Signed-off-by: Ben Pfaff <[email protected]>

blp force-pushed the fxp-round-even-fix branch from daba26f to 7f9fee6 Compare

November 3, 2026 01:03

blp commented Nov 3, 2026

Copy link

Copy Markdown

Member Author

Thanks for the review. I added the SQL-level regression test in CastTests.issue7717 (sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/simple/CastTests.java) and squashed it into the commit. It reads from table columns, so Calcite cannot constant-fold the casts:

Input	`CAST(s AS DECIMAL(5, 0))`	`CAST(d AS TIMESTAMP)`
`'0.6'`, `0.0006`	`1`	`1970-01-01 00:00:00.000001`
`'-0.6'`, `-0.0006`	`-1`	`1969-12-31 23:59:59.999999`
`'0.5'`, `0.0005` (tie)	`0`	`1970-01-01 00:00:00`
`'1.5'`, `0.0015` (tie)	`2`	`1970-01-01 00:00:00.000002`

With only the fix line reverted, the test fails: ±0.6 give 0 and ±0.0006 give the epoch. With the fix, it passes.

-- Claude

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

blp added this pull request to the merge queue

Nov 3, 2026

Merged via the queue into main with commit c9ce238

Nov 3, 2026

1 check passed

Uh oh!

There was an error while loading. Please reload this page.

blp deleted the fxp-round-even-fix branch

November 3, 2026 18:21

Reviewers

mihaibudiu

mihaibudiu approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

blp

Labels

bug

Something isn't working

rust

Pull requests that update Rust code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[fxp] half-even rounding gives 0 instead of ±1 when the truncated quotient is 0

Uh oh!

There was an error while loading. Please reload this page.

2 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.