fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers #14223

nuno-faria · 2025-01-21T12:48:10Z

Previously, when combining UInt64 with any signed integer, the resulting type would be Int64, which would result in lost information. Now, combining UInt64 with a signed integer results in a Decimal(20, 0), which is able to encode all (64-bit) integer types. Thanks @jonahgao for the pointers.

The function bitwise_coercion remains the same, since it's probably not a good idea to introduce decimals when performing bitwise operations. In this case, it converts (UInt64 | _) to UInt64.

Which issue does this PR close?

Closes #14208.

What changes are included in this PR?

Updated binary_numeric_coercion in expr-common/type_coercion/binary.rs.
Added new tests to expr-common/type_coercion/binary.rs.
Updated existing sqllogic tests to use the new coercion.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

… signed integers Previously, when combining UInt64 with any signed integer, the resulting type would be Int64, which would result in lost information. Now, combining UInt64 with a signed integer results in a Decimal(20, 0), which is able to encode all (64-bit) integer types.

jonahgao · 2025-01-21T14:49:25Z

datafusion/expr-common/src/type_coercion/binary.rs

+        // accommodates all values of both types. Note that to avoid information
+        // loss when combining UInt64 with signed integers we use Decimal128(20, 0).
+        (Decimal128(20, 0), _)
+        | (_, Decimal128(20, 0))


This looks not correct. For example, combining Decimal128(20, 0) with Decimal128(30, 0) should not result in Decimal128(20, 0)

I think when both types are decimal they are handled above before this match, when calling the decimal_coercion function.

I rechecked it, and decimal_coercion already covers them in

datafusion/datafusion/expr-common/src/type_coercion/binary.rs

Lines 932 to 940 in 2f28327

fn coerce_numeric_type_to_decimal(numeric_type: &DataType) -> Option<DataType> {

use arrow::datatypes::DataType::*;

// This conversion rule is from spark

// https://github.com/apache/spark/blob/1c81ad20296d34f137238dadd67cc6ae405944eb/sql/catalyst/src/main/scala/org/apache/spark/sql/types/DecimalType.scala#L127

match numeric_type {

Int8 => Some(Decimal128(3, 0)),

Int16 => Some(Decimal128(5, 0)),

Int32 => Some(Decimal128(10, 0)),

Int64 => Some(Decimal128(20, 0)),

Although it doesn't handle unsigned integer types, we can supplement it there, maybe as a follow-up PR.

So I think we shouldn't combine decimal with integer types here because decimal_coercion has already handled it.

I tried that initially but then it would not handle UInt64 and Decimal128(20, 0):

Cannot infer common argument type for comparison operation UInt64 = Decimal128(20, 0)

So maybe it would be best to add new arms to the coerce_numeric_type_to_decimal to include unsigned integers as well?

match numeric_type { Int8 => Some(Decimal128(3, 0)), Int16 => Some(Decimal128(5, 0)), Int32 => Some(Decimal128(10, 0)), Int64 => Some(Decimal128(20, 0)), Float32 => Some(Decimal128(14, 7)), Float64 => Some(Decimal128(30, 15)), _ => None, }

So maybe it would be best to add new arms to the coerce_numeric_type_to_decimal to include unsigned integers as well?

I think so.

jonahgao · 2025-01-21T14:51:12Z

Perhaps we should add a sqllogictest test for #14208.

nuno-faria · 2025-01-21T15:17:01Z

Perhaps we should add a sqllogictest test for #14208.

Done.

…c_type_to_decimal

datafusion/expr-common/src/type_coercion/binary.rs

jonahgao

LGTM, thank you @nuno-faria

alamb · 2025-01-22T23:36:32Z

Thanks again @nuno-faria - it is great to see you contributing ❤️

alamb

Thanks for the contribution @nuno-faria and the review @jonahgao

However, this change doesn't seem like a good one to me

alamb · 2025-01-22T23:38:59Z

datafusion/expr-common/src/type_coercion/binary.rs

-        // for largest signed (signed sixteen-byte integer) and unsigned integer (unsigned sixteen-byte integer)
+        // accommodates all values of both types. Note that to avoid information
+        // loss when combining UInt64 with signed integers we use Decimal128(20, 0).
+        (UInt64, Int64 | Int32 | Int16 | Int8)


I think this has potentially (large) performance implications.

My understanding is that this means that Int64+Int64 will result in (always) a 128bit result?

So even though performing int64+int64 will never overflow, all queries will pay the price of 2x space (and some time) overhead?

No, Int64+Int64 is not affected, it uses mathematics_numerical_coercion.

I did some validation on this PR branch.

DataFusion CLI v44.0.0 > create table test(a bigint, b bigint unsigned) as values(1,1); 0 row(s) fetched. Elapsed 0.008 seconds. > select arrow_typeof(a+b), arrow_typeof(a+a), arrow_typeof(a), arrow_typeof(b) from test; +-------------------------------+-------------------------------+----------------------+----------------------+ | arrow_typeof(test.a + test.b) | arrow_typeof(test.a + test.a) | arrow_typeof(test.a) | arrow_typeof(test.b) | +-------------------------------+-------------------------------+----------------------+----------------------+ | Int64 | Int64 | Int64 | UInt64 | +-------------------------------+-------------------------------+----------------------+----------------------+ 1 row(s) fetched. Elapsed 0.009 seconds.

I really liked this way to verify the behavior. I made a PR with this type of test and verified that the tests still pass with the changes in this PR:

Minor: Add tests for types of arithmetic operator output types #14250

alamb · 2025-01-22T23:40:07Z

datafusion/sqllogictest/test_files/union.slt

 04)------TableScan: aggregate_test_100 projection=[c1, c9]
-05)----Projection: aggregate_test_100.c1, CAST(aggregate_test_100.c3 AS Int64) AS c9
+05)----Projection: aggregate_test_100.c1, CAST(aggregate_test_100.c3 AS Decimal128(20, 0)) AS c9


this seems like a regression to me (there is now 2x the space needed)

This query unions Int16 with UInt64. We need to find a common type that can accommodate all possible values of these two types, such as -1 and u64::MAX. Despite increasing the space, it makes the following query available.

create table t1(a smallint) as values(1); create table t2(a bigint unsigned) as values(10000000000000000000); select * from t1 union select * from t2;

alamb · 2025-01-22T23:41:44Z

datafusion/sqllogictest/test_files/insert.slt

+3
+
+query I rowsort
+select * from unsigned_bigint_test


I agree the case of overflow on * of 2 64 bit numbers is more likely and automatically coercing to Decimal128 may make sense.

However, I would argue that if the user cares about avoiding overflows when doing Intger arithmetic they should use Decimal128 in their input types

Overflow is unexpected in this case, as all these values are valid unsigned bigint.

The problem is that in values (10000000000000000001), (1), 10000000000000000001 is parsed as UInt64, and 1 is parsed as Int64. They were coerced to Int64, which can't accommodate 10000000000000000001.

This is very similar to the union case mentioned above.

alamb

Thank you @nuno-faria and @jonahgao for the explanation. I agree this PR makes sense and my concerns did not apply.

alamb · 2025-01-23T14:38:16Z

datafusion/expr-common/src/type_coercion/binary.rs

-        // for largest signed (signed sixteen-byte integer) and unsigned integer (unsigned sixteen-byte integer)
+        // accommodates all values of both types. Note that to avoid information
+        // loss when combining UInt64 with signed integers we use Decimal128(20, 0).
+        (UInt64, Int64 | Int32 | Int16 | Int8)


I really liked this way to verify the behavior. I made a PR with this type of test and verified that the tests still pass with the changes in this PR:

Minor: Add tests for types of arithmetic operator output types #14250

jonahgao · 2025-01-24T01:29:30Z

Thanks @nuno-faria and @alamb for the review.

ozankabak · 2025-01-24T05:08:08Z

When we combine a UInt64 with a literal value (say 5), we don't accidentally consider the literal value a signed value and do this coercion to decimal, right? That would kill performance in many cases.

jonahgao · 2025-01-24T06:34:48Z

When we combine a UInt64 with a literal value (say 5), we don't accidentally consider the literal value a signed value and do this coercion to decimal, right? That would kill performance in many cases.

We always parse 5 as int64 (see parse_sql_number), so this coercion result will be decimal. The same thing also happens in DuckDB.

v1.1.1-dev319 af39bd0dcf
D create table t(a ubigint);
D select * from t union select 5;
┌────────┐
│   a    │
│ int128 │
├────────┤
│      5 │
└────────┘

This is because, during type coercion, we only consider the data types of the binary operands and do not take their actual data into account. Some binary operations, such as union and comparisons, require that the operands have the same data type, so the coerced type must be a superset of both operand types and therefore can accommodate all possible operand values.

For this case, maybe we can use statistical information to examine the operands' data and perform some physical optimizations.

ozankabak · 2025-01-24T08:28:47Z

This is a problem with a large potential performance impact in many use cases. I do not think it is a good idea to defer its solution with an open-ended timeframe. I see two ways to proceed:

If it is relatively easy, we can do a quick follow-on PR to parse/type literals more intelligently. Maybe always parse as the smallest/narrowest datatype that will hold the constant. We will probably need to somehow decide whether to use a signed type for the constant as well. Coercion will then widen the type if necessary. However, I am not sure if this is possible to do in a straightforward way.
If doing (1) is not easy, we should revert this PR to avoid the performance impact and think of a more holistic solution that includes a mechanism for doing something like (1), or a more involved mechanism utilizing statistics.

I am fine with both ways forward.

alamb · 2025-01-24T09:17:57Z

@ozankabak note the coercion rules @jonahgao refers to above in #14223 (comment) are for UNION, not for example, integer arithmetic)

I am writing some (more) tests to show what is happening

alamb · 2025-01-24T09:25:25Z

Here is a PR that tries to demonstrate more of what is happening:

Add more tests showing coercing behavior with literals #14270

findepi · 2025-01-24T09:28:36Z

If it is relatively easy, we can do a quick follow-on PR to parse/type literals more intelligently. Maybe always parse as the smallest/narrowest datatype that will hold the constant.

Trino parses integer literals as integer type unless they do not fit 32-bit, then they are bigint.
This logic generally makes sense, i don't know what SQL spec or Postgres do.

Smallest possible type may be a step to far. For example 120 + 120 would overflow, if 120 is typed as Int8.

Some other systems (eg Snowflake, PostgreSQL) apparently defer literals typing, making it contextual. We have discussed this for varchar-like literals in date/time argument context, but this approach could perhaps also be applicable to numeric literals.

jonahgao · 2025-01-24T09:50:43Z

Some information may be useful:

This coercion rule applies to union and comparisons (for example, a > b, a <= b), but does not apply to arithmetic operations (for example, a + b). Comparisons in arrow-rs require that the operands have the same type.
Before this PR, we already had similar behavior; for instance, combining UInt32 and Int8 would result in Int64. This PR extends that behavior to UInt64. Without this change, the following queries will fail.

DataFusion CLI v44.0.0
> select 10000000000000000001 > -1;
Arrow error: Cast error: Can't cast value 10000000000000000001 to type Int64

> select 10000000000000000001 union select -1;
External error: External error: Arrow error: Cast error: Can't cast value 10000000000000000001 to type Int64

# use narrowest datatype
> select 10000000000000000001 union select -1::tinyint;
External error: External error: Arrow error: Cast error: Can't cast value 10000000000000000001 to type Int64

Postgres and Spark do not provide unsigned types; DuckDB does, and its behavior is similar to ours when combining an unsigned and a signed integer type.

ozankabak · 2025-01-24T10:37:07Z

Comparison with a literal is a very common pattern. I am still worried.

berkaysynnada · 2025-01-24T12:31:24Z

I think this change brings more harm than it helps. I wrote a simple benchmark:

Table having 100_000 rows, and one column in UInt64
The values are randomly generated between 0 and 100_000_000
The query is SELECT a > -1 FROM random_a

after this commit:

SELECT a > -1
                       time:   [1.3010 ms 1.4096 ms 1.5468 ms]
Found 13 outliers among 100 measurements (13.00%)
 6 (6.00%) high mild
 7 (7.00%) high severe

before this commit:

SELECT a > -1
                        time:   [601.85 µs 604.89 µs 610.50 µs]
                        change: [-60.997% -57.120% -53.557%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  1 (1.00%) low mild
  1 (1.00%) high mild
  3 (3.00%) high severe

I believe we should find a more clever way before the release

jonahgao · 2025-01-24T13:11:21Z

Comparison with a literal is a very common pattern. I am still worried.

I found that unwrap_cast_in_comparison can optimize this case.

DataFusion CLI v44.0.0
create table t(a bigint unsigned);

> explain verbose select a > 1 from t;
| logical_plan after simplify_expressions                    | Projection: CAST(t.a AS Decimal128(20, 0)) > Decimal128(Some(1),20,0) AS t.a > Int64(1)                                |
|                                                            |   TableScan: t                                                                                                         |
| logical_plan after unwrap_cast_in_comparison               | Projection: t.a > UInt64(1) AS t.a > Int64(1)                                                                          |
|                                                            |   TableScan: t                                                                                                         |

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jan 21, 2025

jonahgao reviewed Jan 21, 2025

View reviewed changes

test: Add sqllogictest for apache#14208

d632d7a

nuno-faria added 2 commits January 22, 2025 09:36

refactor: Move unsigned integer and decimal coercion to coerce_numeri…

c21610c

…c_type_to_decimal

fix: Also handle unsigned integers when coercing to Decimal256

befdc43

jonahgao reviewed Jan 22, 2025

View reviewed changes

datafusion/expr-common/src/type_coercion/binary.rs Show resolved Hide resolved

fix: Coerce UInt64 and other unsigned integer to UInt64

bb2c5c0

jonahgao approved these changes Jan 22, 2025

View reviewed changes

alamb reviewed Jan 22, 2025

View reviewed changes

alamb mentioned this pull request Jan 23, 2025

Minor: Add tests for types of arithmetic operator output types #14250

Merged

alamb approved these changes Jan 23, 2025

View reviewed changes

jonahgao merged commit e0f9f65 into apache:main Jan 24, 2025
25 checks passed

alamb mentioned this pull request Jan 24, 2025

Add more tests showing coercing behavior with literals #14270

Open

nuno-faria deleted the uint64_cast_bug branch January 24, 2025 10:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers #14223

fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers #14223

nuno-faria commented Jan 21, 2025

jonahgao Jan 21, 2025

nuno-faria Jan 21, 2025

jonahgao Jan 21, 2025

jonahgao Jan 22, 2025

nuno-faria Jan 22, 2025

jonahgao Jan 22, 2025

nuno-faria Jan 22, 2025

jonahgao commented Jan 21, 2025

nuno-faria commented Jan 21, 2025

jonahgao left a comment

alamb commented Jan 22, 2025

alamb left a comment

alamb Jan 22, 2025

jonahgao Jan 23, 2025

alamb Jan 23, 2025

alamb Jan 22, 2025

jonahgao Jan 23, 2025

alamb Jan 22, 2025

jonahgao Jan 23, 2025

alamb left a comment

alamb Jan 23, 2025

jonahgao commented Jan 24, 2025

ozankabak commented Jan 24, 2025

jonahgao commented Jan 24, 2025

ozankabak commented Jan 24, 2025 •

edited

Loading

alamb commented Jan 24, 2025 •

edited

Loading

alamb commented Jan 24, 2025

findepi commented Jan 24, 2025

jonahgao commented Jan 24, 2025 •

edited

Loading

ozankabak commented Jan 24, 2025

berkaysynnada commented Jan 24, 2025 •

edited

Loading

jonahgao commented Jan 24, 2025

	fn coerce_numeric_type_to_decimal(numeric_type: &DataType) -> Option<DataType> {
	use arrow::datatypes::DataType::*;
	// This conversion rule is from spark
	// https://github.com/apache/spark/blob/1c81ad20296d34f137238dadd67cc6ae405944eb/sql/catalyst/src/main/scala/org/apache/spark/sql/types/DecimalType.scala#L127
	match numeric_type {
	Int8 => Some(Decimal128(3, 0)),
	Int16 => Some(Decimal128(5, 0)),
	Int32 => Some(Decimal128(10, 0)),
	Int64 => Some(Decimal128(20, 0)),

fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers #14223

fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers #14223

Conversation

nuno-faria commented Jan 21, 2025

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgao commented Jan 21, 2025

nuno-faria commented Jan 21, 2025

jonahgao left a comment

Choose a reason for hiding this comment

alamb commented Jan 22, 2025

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgao commented Jan 24, 2025

ozankabak commented Jan 24, 2025

jonahgao commented Jan 24, 2025

ozankabak commented Jan 24, 2025 • edited Loading

alamb commented Jan 24, 2025 • edited Loading

alamb commented Jan 24, 2025

findepi commented Jan 24, 2025

jonahgao commented Jan 24, 2025 • edited Loading

ozankabak commented Jan 24, 2025

berkaysynnada commented Jan 24, 2025 • edited Loading

jonahgao commented Jan 24, 2025

ozankabak commented Jan 24, 2025 •

edited

Loading

alamb commented Jan 24, 2025 •

edited

Loading

jonahgao commented Jan 24, 2025 •

edited

Loading

berkaysynnada commented Jan 24, 2025 •

edited

Loading