Don't silently convert NUMERIC NaN and Infinity to zero - #1070
Open
LiangrunDa wants to merge 1 commit into
Open
LiangrunDa wants to merge 1 commit into
LiangrunDa wants to merge 1 commit into
Conversation
Open
2 tasks
LiangrunDa
force-pushed
the
fix-numeric-special-values
branch
from
August 18, 2026 05:31
56a8b65 to
298504f
Compare
Postgres stores NaN and +/-Infinity as "special" numerics, whose digit array is empty. FromNumeric() converted those to a NumericVar without checking NUMERIC_IS_SPECIAL(), and ConvertDecimal() turns anything with no digits into 0. Reading such a value from a table through the DuckDB execution engine therefore produced a silently wrong result: sum(), avg(), min() and max() over a NUMERIC column containing a NaN all disagreed with Postgres, without a warning. The vendored pg_numeric_c.hpp header documents that this must not happen: "we never convert special numerics to NumericVar form". A DOUBLE can represent all three values, so the NUMERIC-as-DOUBLE conversion now maps them onto their IEEE 754 counterparts. A DECIMAL is a fixed point integer and has no representation for them at all, so that path raises a conversion error instead, the same way a NaN constant already fails today (duckdb#1039). Fixes duckdb#1069
LiangrunDa
force-pushed
the
fix-numeric-special-values
branch
from
August 18, 2026 05:43
298504f to
dfe17e9
Compare
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.
Fixes #1069.
Postgres stores
NaNand±Infinityas "special" numerics, whose digit array is empty.FromNumeric()converted those to aNumericVarwithout checkingNUMERIC_IS_SPECIAL(), andConvertDecimal()turns anything with no digits into0. Reading such a value from a table through the DuckDB execution engine therefore produced a silently wrong result:The vendored
pg_numeric_c.hppheader documents that this must not happen, right aboveNUMERIC_SIGN:What this changes
A
DOUBLEcan represent all three values, so theNUMERIC→DOUBLEconversion (used whenduckdb.convert_unsupported_numeric_to_doubleis on) now maps them onto their IEEE 754 counterparts. Before,NaN,Infinityand-Infinityall came back as0.A
DECIMALis a fixed point integer and has no representation for them at all, so that path raises a conversion error instead of returning a wrong number. This makes a NaN read from a table behave like a NaN constant, which already fails this way today (#1039):I considered rejecting
numeric(p,s)columns at planning time instead, so that such queries would transparently fall back to Postgres. That would be correct in all cases, but it costs DuckDB acceleration for everynumericcolumn whether or not it ever holds a special value, which seemed far too high a price. Happy to reconsider if you disagree.Testing
Adds
numeric_special_valuesto the regression schedule, covering theDECIMALerror, theDOUBLEround-trip, rows without special values, and the float types. There was no NaN or Infinity coverage anywhere intest/regression/ortest/pycheck/before this.Verified on Postgres 14.24 and 18.6: the new test passes and the existing suite stays green (67/67).
Unrelated, but worth flagging since CI will show it:
maincurrently does not build against the tip ofREL_19_STABLE(19beta3).src/vendor/pg_ruleutils_19.creferencesQuery.groupByAllandForPortionOfExpr.range_name, which have since been renamed upstream. This reproduces on an unmodified checkout ofmain, so it is independent of this PR — let me know if you would like a separate issue or PR for it.