Skip to content

Split and deprecate and_or - #24276

Merged
jayzhan211 merged 4 commits into
apache:mainfrom
Nagato-Yuzuru:issue-24208
Aug 20, 2026
Merged

jayzhan211 merged 4 commits into
apache:mainfrom
Nagato-Yuzuru:issue-24208

Conversation

@Nagato-Yuzuru

@Nagato-Yuzuru Nagato-Yuzuru commented Aug 11, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Under Kleene logic, the behavior of and and or is inconsistent with respect to transitivity of ordered.

See #24208 for the counterexamples and the full rules.

What changes are included in this PR?

  • Split SortProperties::and_or into and / or . BinaryExpr::get_properties now calls the matching method per operator.
  • Deprecate and_or and narrow it to what and and or agree on (Singleton/Singleton → Singleton, everything else Unordered). A caller of and_or does 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

@github-actions github-actions Bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Aug 11, 2026
@Nagato-Yuzuru

Copy link
Copy Markdown
Contributor Author

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.

@github-actions

github-actions Bot commented Aug 11, 2026 •

Copy link
Copy Markdown

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
     Cloning apache/main
    Building datafusion-expr-common v54.1.0 (current)
       Built [  20.917s] (current)
     Parsing datafusion-expr-common v54.1.0 (current)
      Parsed [   0.018s] (current)
    Building datafusion-expr-common v54.1.0 (baseline)
       Built [  19.770s] (baseline)
     Parsing datafusion-expr-common v54.1.0 (baseline)
      Parsed [   0.018s] (baseline)
    Checking datafusion-expr-common v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.233s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/inherent_method_missing.ron

Failed in:
  SortProperties::and_or, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/8fed7309b03fa674ae00e0a26ea2ff1075a2dedd/datafusion/expr-common/src/sort_properties.rs:103

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  41.850s] datafusion-expr-common
    Building datafusion-physical-expr v54.1.0 (current)
       Built [  29.810s] (current)
     Parsing datafusion-physical-expr v54.1.0 (current)
      Parsed [   0.048s] (current)
    Building datafusion-physical-expr v54.1.0 (baseline)
       Built [  29.619s] (baseline)
     Parsing datafusion-physical-expr v54.1.0 (baseline)
      Parsed [   0.050s] (baseline)
    Checking datafusion-physical-expr v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.318s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [  61.093s] datafusion-physical-expr
    Building datafusion-sqllogictest v54.1.0 (current)
       Built [  99.022s] (current)
     Parsing datafusion-sqllogictest v54.1.0 (current)
      Parsed [   0.020s] (current)
    Building datafusion-sqllogictest v54.1.0 (baseline)
       Built [ 100.352s] (baseline)
     Parsing datafusion-sqllogictest v54.1.0 (baseline)
      Parsed [   0.022s] (baseline)
    Checking datafusion-sqllogictest v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.093s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [ 202.277s] datafusion-sqllogictest

@github-actions github-actions Bot added the auto detected api change Auto detected API change label Aug 11, 2026
@codecov-commenter

codecov-commenter commented Aug 11, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.26%. Comparing base (3359eab) to head (b8a1c3f).
⚠️ Report is 12 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Aug 12, 2026
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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

  1. Keep the old (incorrect) logic and mark it deprecated to minimize downstream changes required
  2. 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

@Nagato-Yuzuru Nagato-Yuzuru Aug 16, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jayzhan211 It's make sense. I've removed SortProperties::and_or and update the document.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With Agent, I agree rewrite code for breaking change would be a lot easy, we might only need to be careful on behaviour change

@jayzhan211 jayzhan211 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jayzhan211
jayzhan211 added this pull request to the merge queue Aug 20, 2026
Merged via the queue into apache:main with commit ceaef16 Aug 20, 2026
41 checks passed
haochunchang pushed a commit to haochunchang/datafusion that referenced this pull request Oct 28, 2026
## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto detected api change Auto detected API change documentation Improvements or additions to documentation logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) v56.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SortProperties::and_or claims orderings that Kleene AND/OR do not preserve

5 participants