Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Jan 24, 2025
+69 −34
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5f0bd17
fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with…
nuno-faria Jan 21, 2025
d632d7a
test: Add sqllogictest for #14208
nuno-faria Jan 21, 2025
c21610c
refactor: Move unsigned integer and decimal coercion to coerce_numeri…
nuno-faria Jan 22, 2025
befdc43
fix: Also handle unsigned integers when coercing to Decimal256
nuno-faria Jan 22, 2025
bb2c5c0
fix: Coerce UInt64 and other unsigned integer to UInt64
nuno-faria Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 33 additions & 25 deletions datafusion/expr-common/src/type_coercion/binary.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -777,29 +777,19 @@ pub fn binary_numeric_coercion(
(_, Float32) | (Float32, _) => Some(Float32),
// The following match arms encode the following logic: Given the two
// integral types, we choose the narrowest possible integral type that
// accommodates all values of both types. Note that some information
// loss is inevitable when we have a signed type and a `UInt64`, in
// which case we use `Int64`;i.e. the widest signed integral type.

// TODO: For i64 and u64, we can use decimal or float64
// Postgres has no unsigned type :(
// DuckDB v.0.10.0 has double (double precision floating-point number (8 bytes))
// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

| (Int64 | Int32 | Int16 | Int8, UInt64) => Some(Decimal128(20, 0)),
(UInt64, _) | (_, UInt64) => Some(UInt64),
(Int64, _)
| (_, Int64)
| (UInt64, Int8)
| (Int8, UInt64)
| (UInt64, Int16)
| (Int16, UInt64)
| (UInt64, Int32)
| (Int32, UInt64)
| (UInt32, Int8)
| (Int8, UInt32)
| (UInt32, Int16)
| (Int16, UInt32)
| (UInt32, Int32)
| (Int32, UInt32) => Some(Int64),
(UInt64, _) | (_, UInt64) => Some(UInt64),
jonahgao marked this conversation as resolved.
Show resolved Hide resolved
(Int32, _)
| (_, Int32)
| (UInt16, Int16)
Expand Down Expand Up @@ -928,16 +918,16 @@ pub fn get_wider_type(lhs: &DataType, rhs: &DataType) -> Result<DataType> {
}

/// Convert the numeric data type to the decimal data type.
/// Now, we just support the signed integer type and floating-point type.
/// We support signed and unsigned integer types and floating-point type.
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)),
Int8 | UInt8 => Some(Decimal128(3, 0)),
Int16 | UInt16 => Some(Decimal128(5, 0)),
Int32 | UInt32 => Some(Decimal128(10, 0)),
Int64 | UInt64 => Some(Decimal128(20, 0)),
// TODO if we convert the floating-point data to the decimal type, it maybe overflow.
Float32 => Some(Decimal128(14, 7)),
Float64 => Some(Decimal128(30, 15)),
Expand All @@ -946,16 +936,16 @@ fn coerce_numeric_type_to_decimal(numeric_type: &DataType) -> Option<DataType> {
}

/// Convert the numeric data type to the decimal data type.
/// Now, we just support the signed integer type and floating-point type.
/// We support signed and unsigned integer types and floating-point type.
fn coerce_numeric_type_to_decimal256(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(Decimal256(3, 0)),
Int16 => Some(Decimal256(5, 0)),
Int32 => Some(Decimal256(10, 0)),
Int64 => Some(Decimal256(20, 0)),
Int8 | UInt8 => Some(Decimal256(3, 0)),
Int16 | UInt16 => Some(Decimal256(5, 0)),
Int32 | UInt32 => Some(Decimal256(10, 0)),
Int64 | UInt64 => Some(Decimal256(20, 0)),
// TODO if we convert the floating-point data to the decimal type, it maybe overflow.
Float32 => Some(Decimal256(14, 7)),
Float64 => Some(Decimal256(30, 15)),
Expand Down Expand Up @@ -1994,6 +1984,18 @@ mod tests {
Operator::Gt,
DataType::UInt32
);
test_coercion_binary_rule!(
DataType::UInt64,
DataType::UInt8,
Operator::Eq,
DataType::UInt64
);
test_coercion_binary_rule!(
DataType::UInt64,
DataType::Int64,
Operator::Eq,
DataType::Decimal128(20, 0)
);
// numeric/decimal
test_coercion_binary_rule!(
DataType::Int64,
Expand Down Expand Up @@ -2025,6 +2027,12 @@ mod tests {
Operator::GtEq,
DataType::Decimal128(15, 3)
);
test_coercion_binary_rule!(
DataType::UInt64,
DataType::Decimal128(20, 0),
Operator::Eq,
DataType::Decimal128(20, 0)
);

// Binary
test_coercion_binary_rule!(
Expand Down
6 changes: 3 additions & 3 deletions datafusion/sqllogictest/test_files/coalesce.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,13 @@ select
----
2 Int64

# TODO: Got types (i64, u64), casting to decimal or double or even i128 if supported
query IT
# i64 and u64, cast to decimal128(20, 0)
query RT
select
coalesce(2, arrow_cast(3, 'UInt64')),
arrow_typeof(coalesce(2, arrow_cast(3, 'UInt64')));
----
2 Int64
2 Decimal128(20, 0)

statement ok
create table t1 (a bigint, b int, c int) as values (null, null, 1), (null, 2, null);
Expand Down
27 changes: 27 additions & 0 deletions datafusion/sqllogictest/test_files/insert.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,3 +433,30 @@ drop table test_column_defaults

statement error DataFusion error: Error during planning: Column reference is not allowed in the DEFAULT expression : Schema error: No field named a.
create table test_column_defaults(a int, b int default a+1)


# test inserting UInt64 and signed integers into a bigint unsigned column
statement ok
create table unsigned_bigint_test (v bigint unsigned)

query I
insert into unsigned_bigint_test values (10000000000000000000), (18446744073709551615)
----
2

query I
insert into unsigned_bigint_test values (10000000000000000001), (1), (10000000000000000002)
----
3

query I rowsort
select * from unsigned_bigint_test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

----
1
10000000000000000000
10000000000000000001
10000000000000000002
18446744073709551615

statement ok
drop table unsigned_bigint_test
2 changes: 1 addition & 1 deletion datafusion/sqllogictest/test_files/joins.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1828,7 +1828,7 @@ AS VALUES
('BB', 6, 1),
('BB', 6, 1);

query TII
query TIR
select col1, col2, coalesce(sum_col3, 0) as sum_col3
from (select distinct col2 from tbl) AS q1
cross join (select distinct col1 from tbl) AS q2
Expand Down
10 changes: 5 additions & 5 deletions datafusion/sqllogictest/test_files/union.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,23 +413,23 @@ explain SELECT c1, c9 FROM aggregate_test_100 UNION ALL SELECT c1, c3 FROM aggre
logical_plan
01)Sort: aggregate_test_100.c9 DESC NULLS FIRST, fetch=5
02)--Union
03)----Projection: aggregate_test_100.c1, CAST(aggregate_test_100.c9 AS Int64) AS c9
03)----Projection: aggregate_test_100.c1, CAST(aggregate_test_100.c9 AS Decimal128(20, 0)) AS c9
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

06)------TableScan: aggregate_test_100 projection=[c1, c3]
physical_plan
01)SortPreservingMergeExec: [c9@1 DESC], fetch=5
02)--UnionExec
03)----SortExec: TopK(fetch=5), expr=[c9@1 DESC], preserve_partitioning=[true]
04)------ProjectionExec: expr=[c1@0 as c1, CAST(c9@1 AS Int64) as c9]
04)------ProjectionExec: expr=[c1@0 as c1, CAST(c9@1 AS Decimal128(20, 0)) as c9]
05)--------RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=1
06)----------CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/testing/data/csv/aggregate_test_100.csv]]}, projection=[c1, c9], has_header=true
07)----SortExec: TopK(fetch=5), expr=[c9@1 DESC], preserve_partitioning=[true]
08)------ProjectionExec: expr=[c1@0 as c1, CAST(c3@1 AS Int64) as c9]
08)------ProjectionExec: expr=[c1@0 as c1, CAST(c3@1 AS Decimal128(20, 0)) as c9]
09)--------RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=1
10)----------CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/testing/data/csv/aggregate_test_100.csv]]}, projection=[c1, c3], has_header=true

query TI
query TR
SELECT c1, c9 FROM aggregate_test_100 UNION ALL SELECT c1, c3 FROM aggregate_test_100 ORDER BY c9 DESC LIMIT 5
----
c 4268716378
Expand Down
Loading