You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "nuno-faria (via GitHub)" <gi...@apache.org> on 2025/01/21 12:48:12 UTC
[PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
nuno-faria opened a new pull request, #14223:
URL: https://github.com/apache/datafusion/pull/14223
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?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #14208.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes 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?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Yes.
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#discussion_r1926245243
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -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)
Review Comment:
No, `Int64+Int64` is not affected, it uses [mathematics_numerical_coercion](https://github.com/apache/datafusion/blob/440fcb2b6efdf6e6fe2a0ece77612789103df831/datafusion/expr-common/src/type_coercion/binary.rs#L186).
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.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2612059136
> 1. 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2613874994
For the scalar case, like `SELECT a > -1`, consider any `SELECT a > b` where b is constant. I think we could optimize it since we know the value. If the scalar is negative, it can be simplified to `true`. Otherwise, we can rewrite the expression as `min(a, i64::max) > b OR (min(a, i64::max) = i64::max AND b = i64::max)`.
However, for the column case, like `SELECT a > b`, we can only rewrite it to the latter form.
I believe similar optimization rules exist for other comparison operators, given that the type implies the range of the values.
Considering COALESCE and UNION, for the scalar case, we can rewrite the expression given the known value. But for the column case, I think Decimal128 is the only viable option
I quite agree this should be handled in optimizer in general like `unwrap_cast_in_comparison` or physical optimizer rule that based on column statistics.
Another question I would like to know is whether the u64+i64 combination is common in DataFusion? And whether we can avoid this at all. I guess u64 that is larger than i64::max is uncommon, can we aggressively use i64 even though we know it is always positive?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "nuno-faria (via GitHub)" <gi...@apache.org>.
nuno-faria commented on code in PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#discussion_r1925045652
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -777,29 +777,18 @@ 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)
+ | (Int64 | Int32 | Int16 | Int8, UInt64) => Some(Decimal128(20, 0)),
(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),
Review Comment:
Done. I also added a new test to check that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#discussion_r1926121097
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -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)
Review Comment:
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?
##########
datafusion/sqllogictest/test_files/insert.slt:
##########
@@ -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
Review Comment:
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
##########
datafusion/sqllogictest/test_files/union.slt:
##########
@@ -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
Review Comment:
this seems like a regression to me (there is now 2x the space needed)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao merged PR #14223:
URL: https://github.com/apache/datafusion/pull/14223
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2613004105
> 1. 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.
Given that we are talking about coercions in the context of comparisons and unions, I think a flavor/improvement of the idea above may be simple to implement, achieve correctness and avoid the issue with this PR (which is basically incurring TD to fix the performance with no timeframe for the fix).
Let's get more ideas in, and see if there is an even better solution. Once it shows, we can either: Create a PR for it if easy enough, or create an issue describing the approach and revert this if there is no volunteer for it.
Someone from our team will take it on at some point if there is no takers, as the correctness issue is important even though it is a corner case. Thanks for bringing this into our attention @jonahgao
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#discussion_r1923866675
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -777,29 +777,20 @@ 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).
+ (Decimal128(20, 0), _)
+ | (_, Decimal128(20, 0))
Review Comment:
This looks not correct. For example, combining `Decimal128(20, 0)` with `Decimal128(30, 0)` should not result in `Decimal128(20, 0)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2608485205
Thanks again @nuno-faria - it is great to see you contributing โค๏ธ
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2612038896
@ozankabak note the coercion rules @jonahgao refers to above in https://github.com/apache/datafusion/pull/14223#issuecomment-2611707619 are for `UNION`, not for example, integer arithmetic)
I am writing some tests to show what is happening
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2613993556
> > I ran benchmarks with / without this change and did not see any noticable performance difference. See details here
>
> I don't have a detailed knowledge about the benchmarks' scope, but you can try this: https://github.com/synnada-ai/datafusion-upstream/tree/benchmark-cast
>
> That would show the drastic regression.
Got it -- that is comparing a negative number with an unsigned column (`a` is a `UInt64`) so it makes sense that now `random_a` is being cast to `Decimal` for the conversion
```sql
SELECT a > -1 FROM random_a
```
Which turns into
```sql
SELECT CAST(a, Decimal) > Decimal(-1) FROM random_a
```
We could probably fix this particular case with some sort of change in unwrap_cast_in_comparison
Currently unwrap_cast_in_comparison would try to cast `-1` to a UInt and will fail
But we could potentially teach it about negative numbers with unsigned numbers and rewrite the comparison from -1 to 0 (which is still the same)
```sql
SELECT CAST(a, Decimal) > Decimal(0) FROM random_a
```
Which will then get converted back to
```sql
SELECT a > CAST (Decimal(0) as UINT) FROM random_a
```
๐ค
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#discussion_r1925030847
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -777,29 +777,18 @@ 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)
+ | (Int64 | Int32 | Int16 | Int8, UInt64) => Some(Decimal128(20, 0)),
(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),
Review Comment:
We need to keep this arm because it can coerce UInt64 vs UInt8/16/32. The following query returns an incorrect result on this PR.
```sh
DataFusion CLI v44.0.0
> select arrow_typeof(column1) from values(arrow_cast(1, 'UInt8')), (arrow_cast(1, 'UInt64'));
+-----------------------+
| arrow_typeof(column1) |
+-----------------------+
| UInt8 |
| UInt8 |
+-----------------------+
2 row(s) fetched.
Elapsed 0.007 seconds.
> select arrow_typeof(column1) from values(arrow_cast(1, 'UInt8')), (arrow_cast(1000, 'UInt64'));
Arrow error: Cast error: Can't cast value 1000 to type UInt8
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2604947472
Perhaps we should add a sqllogictest test for #14208.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#discussion_r1924987194
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -777,29 +777,20 @@ 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).
+ (Decimal128(20, 0), _)
+ | (_, Decimal128(20, 0))
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#discussion_r1926268514
##########
datafusion/sqllogictest/test_files/insert.slt:
##########
@@ -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
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2613928105
BTW I filed a ticket to track resolving this thread (so that it does't get lost before we release 45.0.0):
- https://github.com/apache/datafusion/issues/14291
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2612053357
Here is a PR that tries to demonstrate more of what is happening:
- https://github.com/apache/datafusion/pull/14270
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "nuno-faria (via GitHub)" <gi...@apache.org>.
nuno-faria commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2605015538
> Perhaps we should add a sqllogictest test for #14208.
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#discussion_r1925030847
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -777,29 +777,18 @@ 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)
+ | (Int64 | Int32 | Int16 | Int8, UInt64) => Some(Decimal128(20, 0)),
(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),
Review Comment:
We need to keep this arm because it can coerce UInt64 vs UInt8/16/32. The following query returns an incorrect result on this PR.
```sh
DataFusion CLI v44.0.0
> select arrow_typeof(column1) from values(arrow_cast(1, 'UInt8')), (arrow_cast(1, 'UInt64'));
+-----------------------+
| arrow_typeof(column1) |
+-----------------------+
| UInt8 |
| UInt8 |
+-----------------------+
2 row(s) fetched.
Elapsed 0.007 seconds.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2611611161
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#discussion_r1927086851
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -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)
Review Comment:
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:
- https://github.com/apache/datafusion/pull/14250
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "nuno-faria (via GitHub)" <gi...@apache.org>.
nuno-faria commented on code in PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#discussion_r1924969888
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -777,29 +777,20 @@ 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).
+ (Decimal128(20, 0), _)
+ | (_, Decimal128(20, 0))
Review Comment:
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?
```rust
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,
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2612102948
Some information may be useful:
1. 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.
2. 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.
```sh
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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "nuno-faria (via GitHub)" <gi...@apache.org>.
nuno-faria commented on code in PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#discussion_r1923882417
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -777,29 +777,20 @@ 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).
+ (Decimal128(20, 0), _)
+ | (_, Decimal128(20, 0))
Review Comment:
I think when both types are decimal they are handled above before this match, when calling the `decimal_coercion` function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#discussion_r1926255432
##########
datafusion/sqllogictest/test_files/union.slt:
##########
@@ -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
Review Comment:
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.
```sh
create table t1(a smallint) as values(1);
create table t2(a bigint unsigned) as values(10000000000000000000);
select * from t1 union select * from t2;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2611352766
Thanks @nuno-faria and @alamb for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2614016503
`UInt64 > -1` is less common than `UInt64 > 1`. In my opinion, ensuring that comparisons between unsigned and signed columns are always available is more important.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2612423408
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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2613968183
I ran benchmarks with / without this change and did not see any noticable performance difference. See details here
- https://github.com/apache/datafusion/pull/14292
I also created a PR with some tests that show the practical impact of this PR on comparisons (I think it is minimal). See
- https://github.com/apache/datafusion/pull/14270
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2613937019
> u64+i64 combination
I mean the comparison_op(u64,i64), coalesce(u64,i64) and union(u64,i64) that use `binary_numeric_coercion` and casted to decimal128. mathematics operation is unaffected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#discussion_r1923910753
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -777,29 +777,20 @@ 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).
+ (Decimal128(20, 0), _)
+ | (_, Decimal128(20, 0))
Review Comment:
I rechecked it, and `decimal_coercion` already covers them in https://github.com/apache/datafusion/blob/2f283277d523404401b206e5b98819a0126341b8/datafusion/expr-common/src/type_coercion/binary.rs#L932-L940
Although it doesn't handle unsigned integer types, we can supplement it there, maybe as a follow-up PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2611707619
> 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](https://github.com/apache/datafusion/blob/a534e853e88365b67363b26a004f6037c2f3a5b0/datafusion/sql/src/expr/value.rs#L80)), 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2613928742
> Another question I would like to know is whether the u64+i64 combination is common in DataFusion? And whether we can avoid this at all. I guess u64 that is larger than i64::max is uncommon, can we aggressively use i64 even though we know it is always positive?
To be clear, a `u64 + u64` in DataFusion still (even after this PR) is `u64` -- we even put a test in to verify this:
https://github.com/apache/datafusion/blob/144ebf9a503d1320a9ebad8689654f38bd18c23f/datafusion/sqllogictest/test_files/operator.slt#L53
I am still not sure I fully understand in what circumstances this change might reduce performance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2613987864
> I ran benchmarks with / without this change and did not see any noticable performance difference. See details here
I don't have a detailed knowledge about the benchmarks' scope, but you can try this:
https://github.com/synnada-ai/datafusion-upstream/tree/benchmark-cast
That would show the drastic regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "nuno-faria (via GitHub)" <gi...@apache.org>.
nuno-faria commented on code in PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#discussion_r1925002712
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -777,29 +777,20 @@ 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).
+ (Decimal128(20, 0), _)
+ | (_, Decimal128(20, 0))
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#discussion_r1924626247
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -777,29 +777,20 @@ 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).
+ (Decimal128(20, 0), _)
+ | (_, Decimal128(20, 0))
Review Comment:
So I think we shouldn't combine decimal with integer types here because `decimal_coercion` has already handled it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(expr-common): Coerce to Decimal(20, 0) when combining UInt64 with signed integers [datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on PR #14223:
URL: https://github.com/apache/datafusion/pull/14223#issuecomment-2612204863
Comparison with a literal is a very common pattern. I am still worried.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org