Skip to content
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

Require sort expressions to be of type Sort #12171

Merged
merged 3 commits into from
Aug 27, 2024
Merged

Require sort expressions to be of type Sort #12171

merged 3 commits into from
Aug 27, 2024

Conversation

findepi
Copy link
Member

@findepi findepi commented Aug 26, 2024 •
edited
Loading

Relates to #1468 (comment)
Prep for #12177

@github-actions github-actions bot added the logical-expr Logical plan and expressions label Aug 26, 2024
@github-actions github-actions bot added the sql SQL Planner label Aug 26, 2024
@findepi findepi changed the title Require sort expressions to be of type Sort in compare_sort_expr Require sort expressions to be of type Sort Aug 26, 2024
alamb
alamb approved these changes Aug 27, 2024
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.

I suppose a panic is better than silent error. However, returning a real error is probably better.

However, given your plans for #12177 I think this is ok for now

Thank you @findepi

@findepi
Copy link
Member Author

findepi commented Aug 27, 2024

However, returning a real error is probably better.

I agree.

In #12177 i am going to introduce a few temporary clones and panics, when splitting commits to make the change reviewable. The idea is that all these go away when we land the whole thing.

@alamb alamb merged commit 4a94356 into apache:main Aug 27, 2024
26 of 27 checks passed
@findepi findepi deleted the findepi/test-sort-expr-is-sort branch August 27, 2024 13:25
@alamb
Copy link
Contributor

alamb commented Sep 5, 2024

🚀 -- thanks again @findepi -- this was epic

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

Assignees
No one assigned
Labels
logical-expr Logical plan and expressions sql SQL Planner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@findepi @alamb