Skip to content

fix(physical-expr): Remove empty constants check when ordering is satisfied #14829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
xudong963 merged 2 commits into from
Feb 24, 2025
Merged

fix(physical-expr): Remove empty constants check when ordering is satisfied #14829

xudong963 merged 2 commits into from
Feb 24, 2025

Conversation

@rkrishn7
Copy link
Contributor

@rkrishn7 rkrishn7 commented Feb 22, 2025

Which issue does this PR close?

Rationale for this change

This PR removes the condition that an inputs constants be empty in order to validate its ordering is satisfied by the target equivalence properties. If the ordering is satisfied, there should be no need to augment it with constants.

Across the failing tests, the lhs contained an ordering requirement while the right hand side did not. Thus, the ordering of the lhs should always satisfy the rhs properties. This was not the case however because both sides contained constants.

What changes are included in this PR?

Are these changes tested?

Failing tests have been updated

Are there any user-facing changes?

N/A

@github-actions github-actions bot added physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Feb 22, 2025
@rkrishn7
Copy link
Contributor Author

rkrishn7 commented Feb 22, 2025

cc @alamb

Let me know if this change makes sense to you!

alamb
alamb approved these changes Feb 23, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @rkrishn7 -- I agree this change makes sense.

Very nice fix of a bug (by deleting code!) 🏆

cc @berkaysynnada

berkaysynnada
berkaysynnada approved these changes Feb 23, 2025
View reviewed changes
Copy link
Contributor

@berkaysynnada berkaysynnada left a comment

Choose a reason for hiding this comment

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

Thank you @rkrishn7. I cannot really remember the need for that constants.is_empty() check. As the tests pass, probably it might be leftover. I was suspicious about the placeholder's equivalence information is set correctly. I checked it and see there is no problem with the union's inputs. So, it is LGTM

@rkrishn7
Copy link
Contributor Author

rkrishn7 commented Feb 24, 2025

Thanks @alamb @berkaysynnada!

Yeah, not too sure why it was there 🤔. For additional context, this also was occurring for UNION ALL, when only one input in the union had an ordering and there were constants on both sides. For example:

> create table t1 (a int);
0 row(s) fetched.
Elapsed 0.003 seconds.

> select a, 1 from t1 union all select 2, 1 from t1 order by 1;
SanityCheckPlan
caused by
Error during planning: Plan: ["SortPreservingMergeExec: [a@0 ASC NULLS LAST]", "  UnionExec", "    SortExec: expr=[a@0 ASC NULLS LAST], preserve_partitioning=[false]", "      ProjectionExec: expr=[CAST(a@0 AS Int64) as a, 1 as Int64(1)]", "        DataSourceExec: partitions=1, partition_sizes=[0]", "    ProjectionExec: expr=[2 as a, 1 as Int64(1)]", "      DataSourceExec: partitions=1, partition_sizes=[0]"] does not satisfy order requirements: [a@0 ASC NULLS LAST]. Child-0 order: []

The tests fixed here should cover this, but happy to add another test for UNION ALL if you both think it makes sense.

@xudong963
Copy link
Member

xudong963 commented Feb 24, 2025

Thanks all

@xudong963 xudong963 merged commit a5cc031 into apache:main Feb 24, 2025
24 checks passed
ozankabak pushed a commit to synnada-ai/datafusion-upstream that referenced this pull request Feb 25, 2025
@rkrishn7 @ozankabak
fix(physical-expr): Remove empty constants check when ordering is sat…
e650f2f 
…isfied (apache#14829)

* fix: Remove empty constants check when ordering is satisfied

* fix: Update failing UNION [ALL] BY NAME SLTs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

@xudong963 xudong963 xudong963 approved these changes

@berkaysynnada berkaysynnada berkaysynnada approved these changes

Assignees

No one assigned

Labels

physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Fix failing UNION [ALL] BY NAME tests due to sanity checker

4 participants

@rkrishn7 @xudong963 @alamb @berkaysynnada