Don't convert unsupported numerics to double by default #795
Conversation
Co-Authored-By: dentiny <dentinyhao@gmail.com>
| @@ -137,6 +138,10 @@ InitGUC() { | |||
| "Allow mixed transactions between DuckDB and Postgres", | |||
| &duckdb_unsafe_allow_mixed_transactions); | |||
|
|
|||
| DefineCustomVariable("duckdb.convert_unsupported_numeric_to_double", | |||
There was a problem hiding this comment.
nit
| DefineCustomVariable("duckdb.convert_unsupported_numeric_to_double", | |
| DefineCustomVariable("duckdb.allow_unsupported_numeric_to_double_conversion", |
There was a problem hiding this comment.
I thought a bit about it, and I think I prefer the original name. So keeping as is.
| * Postgres | ||
| */ | ||
| auto duck_type = pgduckdb::ConvertPostgresToDuckColumnType(column); | ||
| pgduckdb::GetPostgresDuckDBType(duck_type, true); |
There was a problem hiding this comment.
This is a bit too bad but it looks like GetPostgresDuckDBType can throw cpp exceptions.. Here it's fine because pgduckdb_get_tabledef is only called in a method that is robust to it. But we should finish cleaning up our code everywhere to make sure this is less likely to happen ;-)
There was a problem hiding this comment.
Changed the new code to throw cpp exceptions for these errors, at least then we're consistent.
| return duckdb::LogicalType(duckdb::LogicalTypeId::DOUBLE, std::move(extra_type_info)); | ||
| auto precision = numeric_typmod_precision(type_modifier); | ||
| auto scale = numeric_typmod_scale(type_modifier); | ||
| if (type_modifier == -1 || precision < 1 || precision > 38 || scale < 0 || scale > 38 || scale > precision) { |
There was a problem hiding this comment.
Would be great to replace 38 with:
std::numeric_limits<float>::max_exponent10
(also is duckdb::LogicalTypeId::DOUBLE backed by float not a double? that sounds surprising?)
There was a problem hiding this comment.
the 38 has nothing to do with floats, it's because the maximum number of decimal digits that fit into an int128 is 38: https://duckdb.org/docs/stable/sql/data_types/numeric.html#fixed-point-decimals
There was a problem hiding this comment.
Ah yeah that makes more sense :) Is there a DuckDB constant we can use?
There was a problem hiding this comment.
There was a problem hiding this comment.
Given that the number there is 39 and the docs use 38, I decided to simply refer to the docs in a code comment. Also if numerics ever get more precision using the constant you linked wouldn't help in updating it automatically, since that is linked specifically to hugeint_t
22261dc to
68ab6e8
Compare
68ab6e8 to
45c114f
Compare
|
Thank you @JelteF for the help! |
In #795 errors were added when using NUMERIC without a precision. It turns out that these were impossible to avoid for a CTAS command, because we didn't use the precision provided by DuckDB when creating the Postgres table. This fixes that and adds some tests to encode the current behaviour. Related to #795
In #795 errors were added when using NUMERIC without a precision. It turns out that these were impossible to avoid for a CTAS command, because we didn't use the precision provided by DuckDB when creating the Postgres table. This fixes that and adds some tests to encode the current behaviour. Related to #795
This stops converting unsupported NUMERIC sizes to doubles. This behaviour is still supported, but it is now opt-in. This means pg_duckdb won't silently reduce precision. It also starts requiring users to specify precision when using NUMERIC types in a table definition, because the default precision is different between PostgreSQL and DuckDB.
Closes #471