-
Notifications
You must be signed in to change notification settings - Fork 0
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
remove more clones #8
remove more clones #8
Conversation
ff5cf45
to
f1e7274
Compare
}) if is_single_distinct_agg(&aggr_expr)? | ||
&& !contains_grouping_set(&group_expr) => | ||
{ | ||
let group_size = group_expr.len(); |
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.
need to copy the len before moving the expr into the iterator
c02a63a
into
appletreeisyellow:chunchun/stop-copy-in-SingleDistinctToGroupBy
.map(|(i, group_expr)| { | ||
if let Expr::Column(_) = group_expr { | ||
// For Column expressions we can use existing expression as is. | ||
(group_expr.clone(), (group_expr, None)) |
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.
one clone is necessary here
); | ||
let arg = args.swap_remove(0); | ||
|
||
let expr_id = distinct_aggr_exprs.hasher().hash_one(&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.
use HashSet<u64>
with manual hashing instead of HashSet<&Expr>
to avoid borrow checker issues.
.. | ||
}) => { | ||
// is_single_distinct_agg ensure args.len=1 | ||
if *distinct |
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.
old code had redundant conditionals so I refactored it into a single if ... else ...
…he#10527) * chore: merge main and resolve conflict * chore: use less copy * chore: remove clone * remove more clones (#8) * refactor: use HashSet<&Expr> instead of HashSet<String> * refactor: remove more cloning * chore: reduce string allocation Co-authored-by: Adam Curtis <[email protected]> * chore: return internal error instead of panacing * chore: use arg display_name as hash key instead of a hashed value --------- Co-authored-by: Adam Curtis <[email protected]>
Note this PR is to merging to main, but the branch https://github.com/appletreeisyellow/arrow-datafusion/tree/chunchun/stop-copy-in-SingleDistinctToGroupBy which belongs to PR apache#10527