Uh oh!
There was an error while loading. Please reload this page.
fix: exclude precision-losing integer-to-float conversions from CastExpr::check_bigger_cast (#23808) - #23809
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #23809 +/- ##
=======================================
Coverage 80.63% 80.63% =======================================
Files 1091 1091 Lines 370720 370736 +16 Branches 370720 370736 +16 =======================================
+ Hits 298934 298947 +13
Misses 53948 53948 - Partials 17838 17841 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nuno-faria
left a comment
There was a problem hiding this comment.
Thanks @getChan. Since only EquivalenceProperties uses this, it should be ok. However, I think it's better to add some sqllogictests to force this bug. I explored this a bit and here is an example that returns invalid results for the int to float case:
Test data (test.csv):
k,v1,167772172,16777216-- t1 is defined with the sort order, t2 is not> create external table t1 (k int, v int) stored as csv with order (v desc, k desc) location 'test.csv';
> create external table t2 (k int, v int) stored as csv location 'test.csv';
-- the result is wrong, since both v_ are equal the k should be switched>select k, cast(v as float) v_ from t1 order by v_ desc, k desc;
+---+------------+
| k | v_ |
+---+------------+
| 1 | 16777216.0 |
| 2 | 16777216.0 |
+---+------------+-- using the version without the sort works>select k, cast(v as float) v_ from t2 order by v_ desc, k desc;
+---+------------+
| k | v_ |
+---+------------+
| 2 | 16777216.0 |
| 1 | 16777216.0 |
+---+------------+Uh oh!
There was an error while loading. Please reload this page.
48bc262 to
0bc07d5Compare
alamb
left a comment
There was a problem hiding this comment.
Thanks @getChan and @nuno-faria
Uh oh!
There was an error while loading. Please reload this page.
…xpr::check_bigger_cast (apache#23808) (apache#23809) ## Which issue does this PR close? - Closesapache#23808. ## Rationale for this change ref. apache#23807 `CastExpr::check_bigger_cast` is used to determine whether a cast is a widening (order-preserving) conversion. Currently, it classifies `Int32 -> Float32`, `UInt32 -> Float32`, `Int64 -> Float64`, and `UInt64 -> Float64` as widening casts. However, integer-to-float conversions for 32-bit and 64-bit integers lose precision when values exceed the mantissa bit limit (24 bits for `Float32`, 53 bits for `Float64`). For example: `16_777_216_i32 as f32 == 16_777_217_i32 as f32` (both yield 16777216.0f32). Because distinct integer inputs can collapse to the same float output, these casts are not strictly 1-to-1 (injective) and can break suffix sort key ordering in multi-column ordering analysis (e.g. `[CAST(a AS Float32), b]`). ## What changes are included in this PR? - Updated `CastExpr::check_bigger_cast` to exclude precision-losing integer-to-float conversions (`Int32/UInt32 -> Float32` and `Int64/UInt64 -> Float64`). - Added unit test `test_check_bigger_cast_precision_loss` to verify precision-losing casts return `false` while exact conversions (`Int16 -> Float32`, `Int32 -> Float64`, etc.) continue to return `true`. ## Are these changes tested? Yes, new unit test `test_check_bigger_cast_precision_loss` in `cast.rs`. ## Are there any user-facing changes? No breaking API changes. Internal optimizer behavior fix.
Which issue does this PR close?
Rationale for this change
ref. #23807
CastExpr::check_bigger_castis used to determine whether a cast is a widening (order-preserving) conversion. Currently, it classifiesInt32 -> Float32,UInt32 -> Float32,Int64 -> Float64, andUInt64 -> Float64as widening casts.However, integer-to-float conversions for 32-bit and 64-bit integers lose precision when values exceed the mantissa bit limit (24 bits for
Float32, 53 bits forFloat64). For example:16_777_216_i32 as f32 == 16_777_217_i32 as f32(both yield 16777216.0f32).Because distinct integer inputs can collapse to the same float output, these casts are not strictly 1-to-1 (injective) and can break suffix sort key ordering in multi-column ordering analysis (e.g.
[CAST(a AS Float32), b]).What changes are included in this PR?
CastExpr::check_bigger_castto exclude precision-losing integer-to-float conversions (Int32/UInt32 -> Float32andInt64/UInt64 -> Float64).test_check_bigger_cast_precision_lossto verify precision-losing casts returnfalsewhile exact conversions (Int16 -> Float32,Int32 -> Float64, etc.) continue to returntrue.Are these changes tested?
Yes, new unit test
test_check_bigger_cast_precision_lossincast.rs.Are there any user-facing changes?
No breaking API changes. Internal optimizer behavior fix.