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

Add support for ORDER BY on unprojected columns #1415

Merged
merged 7 commits into from
Dec 9, 2021
Merged

Add support for ORDER BY on unprojected columns #1415

merged 7 commits into from
Dec 9, 2021

Conversation

viirya
Copy link
Member

@viirya viirya commented Dec 7, 2021

Which issue does this PR close?

Closes #1372.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the datafusion Changes in the datafusion crate label Dec 7, 2021
@xudong963
Copy link
Member

It'll be better to also add a test in datafusion/tests/sql.rs.

xudong963
xudong963 reviewed Dec 8, 2021
View reviewed changes
Copy link
Member

@xudong963 xudong963 left a comment

Choose a reason for hiding this comment

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

I gave some of my suggestions. Thanks for your contribution, @viirya!

datafusion/src/logical_plan/builder.rs Outdated Show resolved Hide resolved
datafusion/src/logical_plan/builder.rs Outdated Show resolved Hide resolved
datafusion/src/logical_plan/builder.rs Outdated Show resolved Hide resolved
datafusion/src/logical_plan/builder.rs Outdated Show resolved Hide resolved
@viirya
Copy link
Member Author

viirya commented Dec 8, 2021

Thanks @xudong963

houqp
houqp reviewed Dec 8, 2021
View reviewed changes
datafusion/src/logical_plan/builder.rs Outdated Show resolved Hide resolved
datafusion/src/logical_plan/builder.rs Outdated Show resolved Hide resolved
@houqp
Copy link
Member

houqp commented Dec 8, 2021

Thank you @viirya , very cool to see this in action.

@viirya
Copy link
Member Author

viirya commented Dec 8, 2021

@houqp thanks for review. Fixed.

houqp
houqp approved these changes Dec 8, 2021
View reviewed changes
Copy link
Member

@houqp houqp left a comment

Choose a reason for hiding this comment

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

LGTM!

@houqp houqp added the bug Something isn't working label Dec 8, 2021
xudong963
xudong963 approved these changes Dec 8, 2021
View reviewed changes
Copy link
Member

@xudong963 xudong963 left a comment

Choose a reason for hiding this comment

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

Nice work!

datafusion/src/logical_plan/builder.rs
Ok(())
})?;

if missing_cols.is_empty() {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@alamb
Copy link
Contributor

alamb commented Dec 9, 2021

Thanks @viirya !

@alamb alamb merged commit 591932f into apache:master Dec 9, 2021
@Dandandan
Copy link
Contributor

Nice 🎉

alamb
alamb reviewed Dec 9, 2021
View reviewed changes
datafusion/tests/sql.rs
let mut ctx = ExecutionContext::new();
register_alltypes_parquet(&mut ctx).await;
// execute the query
let sql = "SELECT id FROM alltypes_plain ORDER BY int_col, double_col";
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@viirya
Copy link
Member Author

viirya commented Dec 9, 2021

Thank you @houqp @xudong963 @alamb @Dandandan !

@alamb alamb mentioned this pull request Dec 9, 2021
Merged
@alamb alamb changed the title Support sort on unprojected columns Add support for ORDER BY on unprojected columns Feb 10, 2022
@alamb alamb added enhancement New feature or request and removed bug Something isn't working labels Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb left review comments

@xudong963 xudong963 xudong963 approved these changes

@houqp houqp houqp approved these changes

Assignees
No one assigned
Labels
datafusion Changes in the datafusion crate enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Sort on unprojected columns
5 participants
@viirya @xudong963 @houqp @alamb @Dandandan