ScalarUdfImpl::strictly_order_preserving: Allow expression to report whether they keep the same ordering of the input - #23807
Conversation
a.cmp(b) == f(a).cmp(f(b)) opening up for more optimization
e0c40b1 to
7305547
Compare
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
a.cmp(b) == f(a).cmp(f(b)) opening up for more optimizationa.cmp(b) == f(a).cmp(f(b))) opening up for more optimization
| if is_order_preserving_cast_family(&child.range.data_type(), target_type) { | ||
| Ok(child.clone().with_range(unbounded)) | ||
| let source_type = child.range.data_type(); | ||
| // A widening cast is additionally one-to-one, so it is strictly |
There was a problem hiding this comment.
I may not have full context, so please feel free to ignore this if it's a misunderstanding.
CastExpr::check_bigger_cast includes Int32 -> Float32 and Int64 -> Float64. Due to float mantissa limits, large integers lose precision and can collapse to the same float value (e.g. 16_777_216_i32 as f32 == 16_777_217_i32 as f32), so they are not strictly 1-to-1
There was a problem hiding this comment.
You are right, unfortunately this moved from here (so no logic change has been made):
| preserves_lex_ordering: value.preserves_lex_ordering, | ||
| // Not carried over the FFI boundary (it would break the ABI); | ||
| // stay conservative. | ||
| strictly_order_preserving: false, |
There was a problem hiding this comment.
Should I make breaking change in FFI (by adding the field) while I'm here to avoid 2 breaking changes? I'm not really familiar with this module
There was a problem hiding this comment.
I've read .ai/skills/datafusion-ffi/SKILL.md (ironic) and saw that it is ok to make breaking change, just not release them in a patch version
There was a problem hiding this comment.
so I added the field here
| if is_order_preserving_cast_family(&child.range.data_type(), target_type) { | ||
| Ok(child.clone().with_range(unbounded)) | ||
| let source_type = child.range.data_type(); | ||
| // A widening cast is additionally one-to-one, so it is strictly |
There was a problem hiding this comment.
You are right, unfortunately this moved from here (so no logic change has been made):
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23807 +/- ##
==========================================
- Coverage 80.66% 80.66% -0.01%
==========================================
Files 1094 1095 +1
Lines 371671 372376 +705
Branches 371671 372376 +705
==========================================
+ Hits 299817 300380 +563
- Misses 53958 54067 +109
- Partials 17896 17929 +33 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alamb
left a comment
There was a problem hiding this comment.
Thank you @rluvaton -- this idea here seems sound to me
Can we get some sql logic / explain test that show some of the optimizations that this allows for (that preserves_lex_ordering does not)?
I think having those examples would make sure we have a good / minimal API to build on -- without such "end visible facing" tests I fear we may implement some features that are not ever used
| Ok(false) | ||
| } | ||
|
|
||
| /// Returns true if the function is strictly order-preserving with respect |
There was a problem hiding this comment.
How is this related to the output_ordering method? I am wondering if we can extend that rather tan introduce a new method
There was a problem hiding this comment.
output_ordering does not tell you whether you can reuse the existing ordering when you have multiple order by keys (with how ties are handled)
| /// | ||
| /// This is not simply a stricter [`Self::preserves_lex_ordering`] — the | ||
| /// two properties also assume different input orderings, and with | ||
| /// multiple ordered inputs neither implies the other. Monotone but |
There was a problem hiding this comment.
It would have helped me to move these examples up to the top of the docs
| preserves_lex_ordering: value.preserves_lex_ordering, | ||
| // Not carried over the FFI boundary (it would break the ABI); | ||
| // stay conservative. | ||
| strictly_order_preserving: false, |
I did not want to add uses of this api in this PR to reduce the amount of code to review, do you still want me to do so? |
|
…d-ordering # Conflicts: # datafusion/expr-common/src/sort_properties.rs # datafusion/expr/src/udf.rs
|
@alamb can you please take another look? |
…xpr::check_bigger_cast (apache#23808) (apache#23809) ## Which issue does this PR close? - Closes apache#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.
|
|
||
| /// Returns true if the function preserves lexicographical ordering based on | ||
| /// the input ordering. | ||
| /// |
There was a problem hiding this comment.
I think it would be ok to keep these descriptions short and refer readers to the very nice SortProperties description
Eg. something like
| /// | |
| /// See [`ExprProperties::preserves_lex_ordering`] for more details |
| /// | ||
| /// # Examples | ||
| /// | ||
| /// * `from_unixtime(a)` returns `true`: it reinterprets the input integer | ||
| /// as a timestamp without changing the value, so distinct inputs yield | ||
| /// distinct, identically-ordered outputs. | ||
| /// * `floor(a)` and `date_trunc('day', a)` must return `false`: they are | ||
| /// monotone, but collapse distinct inputs into equal outputs. | ||
| /// * An addition over ordered, overflow-free inputs may return `true` | ||
| /// even though it does not preserve *lexicographical* ordering. | ||
| /// | ||
| /// # Definition | ||
| /// | ||
| /// Assuming the `Ordered` inputs advance simultaneously (component-wise, | ||
| /// i.e. all of them are sorted in the data), the output is ordered in the | ||
| /// same direction, equal outputs can only result from equal values of | ||
| /// those inputs (i.e. the mapping is one-to-one), and nulls map to nulls. | ||
| /// | ||
| /// This is not simply a stricter [`Self::preserves_lex_ordering`] - the | ||
| /// two properties also assume different input orderings, and with | ||
| /// multiple ordered inputs neither implies the other (see the examples | ||
| /// above). See [`ExprProperties::strictly_order_preserving`] for a | ||
| /// detailed comparison. | ||
| /// | ||
| /// # Relationship to [`Self::output_ordering`] | ||
| /// | ||
| /// [`Self::output_ordering`] describes *whether and in which direction* | ||
| /// the output is ordered, which justifies using the expression as the | ||
| /// **last** (or only) sort key. This property is an additional, | ||
| /// independent claim of injectivity that justifies keeping the **suffix** | ||
| /// sort keys as well: optimizers rely on it to substitute a sort key with | ||
| /// an expression computed from it - if data is sorted by `[x, y]`, it is | ||
| /// also sorted by `[expr(x), y]`, which only holds when equal `expr(x)` | ||
| /// values imply equal `x` values. | ||
| /// | ||
| /// When in doubt, return `false` (the default). The `inputs` properties | ||
| /// allow conditional answers, e.g. based on the ranges of the inputs. | ||
| fn strictly_order_preserving(&self, _inputs: &[ExprProperties]) -> Result<bool> { | ||
| Ok(false) | ||
| } |
There was a problem hiding this comment.
Similarly I think here it would be ok to point back to ExprProperties to make sure the text stays consistent
| /// | |
| /// # Examples | |
| /// | |
| /// * `from_unixtime(a)` returns `true`: it reinterprets the input integer | |
| /// as a timestamp without changing the value, so distinct inputs yield | |
| /// distinct, identically-ordered outputs. | |
| /// * `floor(a)` and `date_trunc('day', a)` must return `false`: they are | |
| /// monotone, but collapse distinct inputs into equal outputs. | |
| /// * An addition over ordered, overflow-free inputs may return `true` | |
| /// even though it does not preserve *lexicographical* ordering. | |
| /// | |
| /// # Definition | |
| /// | |
| /// Assuming the `Ordered` inputs advance simultaneously (component-wise, | |
| /// i.e. all of them are sorted in the data), the output is ordered in the | |
| /// same direction, equal outputs can only result from equal values of | |
| /// those inputs (i.e. the mapping is one-to-one), and nulls map to nulls. | |
| /// | |
| /// This is not simply a stricter [`Self::preserves_lex_ordering`] - the | |
| /// two properties also assume different input orderings, and with | |
| /// multiple ordered inputs neither implies the other (see the examples | |
| /// above). See [`ExprProperties::strictly_order_preserving`] for a | |
| /// detailed comparison. | |
| /// | |
| /// # Relationship to [`Self::output_ordering`] | |
| /// | |
| /// [`Self::output_ordering`] describes *whether and in which direction* | |
| /// the output is ordered, which justifies using the expression as the | |
| /// **last** (or only) sort key. This property is an additional, | |
| /// independent claim of injectivity that justifies keeping the **suffix** | |
| /// sort keys as well: optimizers rely on it to substitute a sort key with | |
| /// an expression computed from it - if data is sorted by `[x, y]`, it is | |
| /// also sorted by `[expr(x), y]`, which only holds when equal `expr(x)` | |
| /// values imply equal `x` values. | |
| /// | |
| /// When in doubt, return `false` (the default). The `inputs` properties | |
| /// allow conditional answers, e.g. based on the ranges of the inputs. | |
| fn strictly_order_preserving(&self, _inputs: &[ExprProperties]) -> Result<bool> { | |
| Ok(false) | |
| } | |
| /// | |
| /// See [`ExprProperties::strictly_order_preserving`] for more details | |
| fn strictly_order_preserving(&self, _inputs: &[ExprProperties]) -> Result<bool> { | |
| Ok(false) | |
| } |
| } | ||
|
|
||
| fn strictly_order_preserving(&self, _inputs: &[ExprProperties]) -> Result<bool> { | ||
| // `from_unixtime` stores the input's exact `Int64` value as a |
| let col_b = col("b", &schema)?; | ||
| let asc = SortOptions::default(); | ||
| let ordering = vec![ | ||
| PhysicalSortExpr::new(Arc::clone(&col_a), asc), |
There was a problem hiding this comment.
We could probably reduce the text here (and make it easier to read) maybe by creating
let ordering_a_b = PhysicalSortExpr::new(..., asc),?
| Arc::clone(&narrowing), | ||
| asc | ||
| )])?); | ||
| assert!(!eq_properties.ordering_satisfy(vec![ |
There was a problem hiding this comment.
It does still satisfy col_a I think -- it might be nice to add an assert here that it does satisfy
a.cmp(b) == f(a).cmp(f(b))) opening up for more optimizationScalarUdfImpl::strictly_order_preserving: Allow expression to report whether they keep the same ordering of the input
Which issue does this PR close?
Related to:
Rationale for this change
To be able to keep the same sorting order allowing for more optimizations
Now comet own
castimplementation can be recognized as not modifying sort order in the same cases that datafusion cast does.What changes are included in this PR?
added
strictly_order_preservingproperty toExprProperties+ varius other placesand replaced the hard coded logic for cast (
substitute_cast_ordering) about keeping input order with more generic approach that now any expression can implement and have the same advantage of sort eliminationalso marked from_unixtime as keeping ordering to show a case of this optimization
Are these changes tested?
yes
Are there any user-facing changes?
yes, breaking change, added
strictly_order_preservingproperty toExprPropertiesand toFFI_ExprPropertiesthis property means that given expression
fand 2 values from the input columnaandbthe following variants are kept:a.cmp(b) == f(a).cmp(f(b))Example of satisfying expression:
cast(col_a as BIGINT)wherecol_aisINTit is keeping the propertiesExample of not satisfying:
floor- floor can notarray_repeat(my_col, 2)which might look like at first glance as keeping the property as well but in fact it does not.the reason is that
array_repeat(null, 2)will output list of 2 nulls which breaks the 2nd property that nulls must be kept as nullshow to migrate:
Option 1 - keeping the old behavior (safest but least performant)
set
strictly_order_preservingto falseOption 2 - Using the new optimization that this opens up:
set
strictly_order_preservingonly if the expression keep both variants