-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
perf: Drop RowConverter from GroupOrderingPartial #14566
base: main
Are you sure you want to change the base?
Conversation
d7e7376
to
a7324c7
Compare
Micro-benchmark results
|
cccc8ea
to
0d778f5
Compare
0d778f5
to
ec21860
Compare
Thank you, this looks good to me. Let's get the CI fixed. https://github.com/apache/datafusion/blob/main/datafusion/core/tests/fuzz_cases/aggregate_fuzz.rs should have good coverage for edge cases in group by with partial ordering, so I think this change is safe |
ec21860
to
2961710
Compare
Thank you for taking the time to review! |
impl GroupOrderingPartial { | ||
pub fn try_new( | ||
input_schema: &Schema, | ||
_input_schema: &Schema, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not remove this arg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way we can avoid API change, though I don't have a strong preference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can add a comment or something as a follow on pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, the input_schema
argument in order::GroupOrdering::try_new
will also be unnecessary
datafusion/datafusion/physical-plan/src/aggregates/order/mod.rs
Lines 43 to 58 in 1626c00
impl GroupOrdering { | |
/// Create a `GroupOrdering` for the specified ordering | |
pub fn try_new( | |
input_schema: &Schema, | |
mode: &InputOrderMode, | |
ordering: &LexOrdering, | |
) -> Result<Self> { | |
match mode { | |
InputOrderMode::Linear => Ok(GroupOrdering::None), | |
InputOrderMode::PartiallySorted(order_indices) => { | |
GroupOrderingPartial::try_new(input_schema, order_indices, ordering) | |
.map(GroupOrdering::Partial) | |
} | |
InputOrderMode::Sorted => Ok(GroupOrdering::Full(GroupOrderingFull::new())), | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a TODO comment to the code
Which issue does this PR close?
Rationale for this change
Faster is better?
What changes are included in this PR?
Are these changes tested?
Yes, added a new test.
Are there any user-facing changes?
No.
(The input_schema argument of GroupOrderingPartial::try_new is no longer used and can be removed, this is not included in this PR)