Skip to content

Conversation

@akurmustafa
Copy link
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

Minor changes in the Sort code to improve readability.

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the physical-expr Changes to the physical-expr crates label Sep 27, 2024
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 @akurmustafa -- this looks like a very nice cange to me

👋 it is also great to see you back making PRs ❤️

.iter()
.map(|c| take(c.as_ref(), &indices, None))
.collect::<Result<_, _>>()?;
let columns = get_arrayref_at_indices(batch.columns(), &indices)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Maybe we should rename get_arrayref_at_indices to take_arrays or something 🤔 (as a follow on PR) to be more consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense to me

@akurmustafa akurmustafa merged commit 84e9ce8 into apache:main Sep 27, 2024
@akurmustafa akurmustafa deleted the simplifications/sort branch September 27, 2024 15:43
@alamb
Copy link
Contributor

alamb commented Sep 29, 2024

Thanks @akurmustafa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants