Repository navigation
Split and deprecate and_or - #24276
Conversation
|
I'm not sure if the patch will into version 55.0. It looks like it's about to be released. I'd be happy to update the deprecation description and add breaking change documentation. |
|
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24276 +/- ##
==========================================
+ Coverage 81.23% 81.26% +0.02%
==========================================
Files 1111 1112 +1
Lines 390012 391661 +1649
Branches 390012 391661 +1649
==========================================
+ Hits 316815 318268 +1453
- Misses 54578 54683 +105
- Partials 18619 18710 +91 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| since = "55.0.0", | ||
| note = "`AND` and `OR` propagate orderings differently under three-valued logic; use `and` or `or`" | ||
| )] | ||
| pub fn and_or(&self, rhs: &Self) -> Self { |
There was a problem hiding this comment.
I think we could remove this method entirely because this is breaking change, downstream project need to change their code too.
@alamb What is the prefer breaking change policy now, do you think we can remove this method and then force to change to and and or for downstream project?
There was a problem hiding this comment.
I agree it doesn't seem like a good experience for downstream users to both change a method AND deprecate it. I would suggest either:
- Keep the old (incorrect) logic and mark it deprecated to minimize downstream changes required
- Remove the method entirely.
I think in general, we have tried to leave methods deprecated for a while to help people migrate as described here - https://datafusion.apache.org/contributor-guide/api-health.html#when-to-make-breaking-api-changes
Though with agents maybe it is less important
There was a problem hiding this comment.
@jayzhan211 It's make sense. I've removed SortProperties::and_or and update the document.
There was a problem hiding this comment.
With Agent, I agree rewrite code for breaking change would be a lot easy, we might only need to be careful on behaviour change
API change: `SortProperties::and_or` is deprecated.
2ad0363 to
018477d
Compare
b91e96e to
b8a1c3f
Compare
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @Nagato-Yuzuru
## 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 apache#123` indicates that this PR will close issue apache#123. --> Part of apache#24243 ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. Please explain the problem you are trying to solve in terms of the user-visible behavior, rather than the implementation. For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the implementation. "COUNT(DISTINCT) returns wrong results when the column contains nulls" is the user-visible problem. --> The equivalence fuzz tests never generated NULLs. So they could not catch NULL-ordering bugs in `SortProperties` propagation such as apache#11596 and apache#24208. ## 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. --> - Generated columns contain NULLs at configurable rates (`NULL_PCTS = [0.0, 0.1, 0.5]`), crossed with every seed; the schema is nullable only when the rate is non-zero. - `create_random_schema` picks random `SortOptions` for each ordering key. - Requirements with random `SortOptions` are checked only for soundness: if `ordering_satisfy` accepts an ordering, the data must be sorted by it. The converse is not checked, since one batch can be sorted by coincidence (e.g. NULLs of independent columns landing in the same rows). For the same reason, `contains_overflowable_arithmetic` is renamed to `contains_conservative_ordering_op` and also covers comparisons and `AND`/`OR`. ## What is the testing strategy for this PR? <!-- 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 Briefly describe how this PR is tested, and point to the specific tests you added. For example: 'This new feature is covered by the `sqllogictest` cases added in `foo.slt`'. If this PR does not add tests, explain why. For example, if the change is already covered by existing tests, please mention it. You should also check the `codecov` bot reply on this PR to confirm the changed code is exercised. --> Test-only change. Verified locally that reverting the fixes from apache#24276 and apache#24206 makes `test_ordering_satisfy_with_equivalence_complex_random` fail, and that it passes on main: ```bash git revert -n ceaef16 4042812 cargo test -p datafusion --test fuzz --features extended_tests equivalence # test_ordering_satisfy_with_equivalence_complex_random fails ``` ## 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
Which issue does this PR close?
Rationale for this change
Under Kleene logic, the behavior of
andandoris inconsistent with respect to transitivity of ordered.See #24208 for the counterexamples and the full rules.
What changes are included in this PR?
SortProperties::and_orintoand/or.BinaryExpr::get_propertiesnow calls the matching method per operator.and_orand narrow it to whatandandoragree on (Singleton/Singleton→Singleton, everything elseUnordered). A caller ofand_ordoes not say which operator it means, so any stronger claim is wrong for one of them. The previous behavior is this bug.Are these changes tested?
Yes
Are there any user-facing changes?
Yes. Split and deprecate
and_or