Skip to content

Fix PartialOrd for logical plan nodes and expressions - #17438

Merged
findepi merged 3 commits into
apache:mainfrom
findepi:findepi/fix-partialord-for-ddl-dml-38b9ac
Sep 9, 2025
Merged

Fix PartialOrd for logical plan nodes and expressions#17438
findepi merged 3 commits into
apache:mainfrom
findepi:findepi/fix-partialord-for-ddl-dml-38b9ac

Conversation

@findepi

@findepifindepi commented Sep 5, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Before the changes, PartialOrd could return Some(Equal) for two values that are not equal in PartialEq sense. This is violation of PartialOrd contract.

The fix is to consult eq inside ord implementation. If ord thinks two instances are equal, but they are not equal in Eq sense, they are considered incomparable.

@findepi
Fix PartialOrd for DDL & DML
43c0cf2
Before the changes, `PartialOrd` could return `Some(Equal)` for two
values that are not equal in `PartialEq` sense. This is violation of
`PartialOrd` contract.
The fix is to consult eq inside ord implementation. If ord thinks two
instances are equal, but they are not equal in Eq sense, they are
considered incomparable.
@github-actionsgithub-actionsBot added the logical-expr Logical plan and expressions label Sep 5, 2025
@findepi
Fix PartialOrd for logical plan nodes and expressions
c6252f4
Before the changes, `PartialOrd` could return `Some(Equal)` for two
values that are not equal in `PartialEq` sense. This is violation of
`PartialOrd` contract.
The fix is to consult eq inside ord implementation. If ord thinks two
instances are equal, but they are not equal in Eq sense, they are
considered incomparable.
@findepifindepi changed the title Fix PartialOrd for DDL & DMLFix PartialOrd for logical plan nodes and expressionsSep 5, 2025
@github-actionsgithub-actionsBot added optimizer Optimizer rules substrait Changes to the substrait crate labels Sep 5, 2025
alamb
alamb reviewed Sep 5, 2025
View reviewed changes

@alambalamb 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.

Thank you @findepi

I think we can make this faster

Comment threaddatafusion/expr/src/logical_plan/plan.rs
@findepi
findepi requested a review from alambSeptember 8, 2025 13:05
@alambalamb mentioned this pull request Sep 8, 2025
Open
alamb
alamb approved these changes Sep 8, 2025
View reviewed changes
Comment threaddatafusion/expr/src/expr.rs
@findepi
findepi merged commit 0c7d830 into apache:mainSep 9, 2025
28 checks passed
@findepi
findepi deleted the findepi/fix-partialord-for-ddl-dml-38b9ac branch September 9, 2025 18:00
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb approved these changes
@timsaucertimsaucerAwaiting requested review from timsaucer

Assignees

No one assigned

Labels

logical-exprLogical plan and expressionsoptimizerOptimizer rulessubstraitChanges to the substrait crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Incorrect implementation of PartialOrd for ScalarUDF, WindowUDF and AggregateUDF

2 participants

@findepi@alamb