Fix silent precision loss on unconstrained Postgres numeric columns - #720
Fix silent precision loss on unconstrained Postgres numeric columns#720NubeDev wants to merge 2 commits into
Conversation
An unconstrained `numeric` column has no fixed scale, but the Arrow column needs one. The row path took it from the first non-null row and then `rescale`d every later value down to it, and `Decimal::rescale` rounds: `20, 17.685, 15.334` came back as `20, 18, 15`. Reordering the same values made them exact, so the corruption depended on row order. Aggregates (`max`, `avg`, arithmetic over `numeric`) return exactly this unconstrained type, so they were the common exposure. The scale is now pinned to `Decimal128(38, 20)` when the column declares none — the same scale schema inference already assigns to a bare `numeric`, so the row path and the schema path agree, and the result no longer depends on which row arrives first or how rows are batched. Declared scales (`numeric(10,3)`) keep taking their scale from the projected schema as before. Values are widened to the column scale by exact i128 arithmetic instead of `Decimal::rescale`, which rounds when narrowing and silently refuses to widen past its own 96-bit mantissa. Anything that cannot be represented exactly at the column scale is now an error rather than a quietly rounded value. `numeric[]` took its scale from the first row's widest element and is fixed the same way. The regression test's fixture is deliberately ordered whole-number first (`20, 17.685, 15.334`); ordered the other way it passes against the unfixed code. Fixes datafusion-contrib#719 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bjchambers
left a comment
There was a problem hiding this comment.
Thanks for the bug report and the fix. A few minor comments / questions.
| source: arrow::error::ArrowError, | ||
| }, | ||
|
|
||
| #[snafu(display( |
There was a problem hiding this comment.
Can we put the column name in these two errors? It would make it easier for a user to understand and address.
| /// | ||
| /// This matches the scale schema inference already assigns to a bare `numeric` | ||
| /// (`Decimal128(38, 20)`), so the row path and the schema path agree. | ||
| const DEFAULT_UNCONSTRAINED_NUMERIC_SCALE: u32 = 20; |
There was a problem hiding this comment.
The constant should be shared between schema inference and this file to ensure they stay in sync.
… schema inference
`DecimalExceedsColumnScale` and `DecimalOverflowsColumnScale` now carry the column
name, so a user hitting either knows which column of their query is at fault rather
than only the offending value. The overflow message also reports the precision it
could not fit rather than hardcoding 38 in the text.
The default precision and scale for a `numeric` that declares none now live beside
schema inference as `schema::DEFAULT_NUMERIC_{PRECISION,SCALE}`, and both
`parse_numeric_type` and the row-decode path read them — the two agreeing is what
makes the fix work, so they can no longer drift apart silently.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks for the review — both addressed in 676ef65. Column name in the errors. The overflow message also reports the precision it could not fit rather than hardcoding 38 in the text. The unit test asserts the column name is present in both. Shared constant. Re-ran here: |
Which issue does this PR close?
Closes #719.
Rationale for this change
An unconstrained Postgres
numericcolumn has no fixed scale — each row may carry a different number of decimal places — but the Arrow column needs a single scale.rows_to_arrowtook that scale from the first non-null row and thenDecimal::rescaled every later value down to it.rescalerounds, so longer values were silently corrupted:The same values in a different order came back exact, and a single-row result is always exact, so the loss survives casual testing.
max(),min(),avg()and arithmetic over anumericcolumn all return this unconstrained type, so aggregates were the common exposure. Columns with a declared scale (numeric(10,3)) were unaffected — they take the schema-driven path.What changes are included in this PR?
numericcolumn that declares none is pinned toDecimal128(38, 20)instead of being inferred from the first row. That is the same scale schema inference already assigns to a barenumeric, so the row path and the schema path now agree, and the result no longer depends on row order or on how rows are chunked into batches.i128arithmetic (decimal_to_i128_mantissa) rather thanDecimal::rescale, which rounds when narrowing and silently refuses to widen past its own 96-bit mantissa. A value that cannot be represented exactly at the column scale is now an error (DecimalExceedsColumnScale/DecimalOverflowsColumnScale) instead of a quietly rounded value.numeric[]derived its scale from the first row's widest element and had the same order dependence; it is fixed the same way.Are there any user-facing changes?
Yes, and they are intended:
numericcolumns now always come back asDecimal128(38, 20)rather than at whichever scale the first row happened to have. Values that were previously rounded are now exact.numericvalue with more decimal places than the column's scale is rejected with an error instead of being silently rounded. Previously this loss was invisible.No public API changes.
Testing
core/tests/postgres/mod.rs:test_postgres_unconstrained_numeric_precisionruns the issue's repro throughquery_arrowwith no projected schema and asserts both the column scale and every value. The fixture is deliberately ordered whole-number first (20, 17.685, 15.334) — ordered the other way it passes against the unfixed code. Verified failing before the fix (left: 0, right: 20, i.e. the zero-decimal worst case) and passing after.crates/postgres/src/arrow_sql_gen/mod.rs: unit tests fordecimal_to_i128_mantissacovering exact widening, an already-matching declared scale, refusal to round, and 128-bit overflow.cargo fmt --all -- --check,cargo clippy -p datafusion-table-providers-postgres --all-features -- -D warnings,cargo test -p datafusion-table-providers-postgres --lib(57 passed), and the full Docker-backed Postgres integration suitecargo test -p datafusion-table-providers --features postgres --test integration postgres(21 passed) all clean locally.🤖 Generated with Claude Code