Omkar-Wagholikar opened a new issue, #26756: URL: https://github.com/apache/datafusion/issues/26756
# Is your feature request related to a problem or challenge? Part of #19795 (Improve query planning speed). While browsing through the analyzer and simplifier code, I found a couple places that deep-clone owned data then discard the original. Importantly, these are not Arc clones, so each one copies real data (thus costlier) on every plan that reaches that code. These are the exact findings: - `exclude_using_columns` (datafusion/expr/src/utils.rs:412) builds a set with plan.schema().columns().iter().cloned().collect(). DFSchema::columns() already returns a fresh Vec<Column>, so every Column is cloned a second time. Thus a small change, along the lines of this can provide a some level of performance improvements as well as reduce the memory footprint of the code at run time - `TypeCoercion::coerce_dml` (datafusion/optimizer/src/analyzer/type_coercion.rs:346) deep-clones the whole MergeIntoOp, including all its clauses, then rebuilds the Box. It also does *value = value.clone().cast_to(..) for each assignment and insert value. - The Between rewrite and as_inlist in datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs use *boxed.clone(), which allocates a new Box only to unwrap it. Possibly related earlier work: #5157, #9637 and #18503 (this one is closed). This looks like a pattern that will keep recurring. # Describe the solution you'd like A small, behavior-preserving cleanup that removes these clones: - Move values instead of cloning them where the original isn't used afterwards (into_iter(), std::mem::take, editing through &mut). - Use x.as_ref().clone() instead of *x.clone() for boxed values. - Measure with cargo bench --profile=profiling --bench sql_planner and report the result: [fill in numbers before and after, or state that no difference was measurable]. I'd do this as one PR, covering the cases above. # Describe alternatives you've considered - Leave it as is, since none of these shows up in current profiles. [Edit this after you have benchmark data.] - Larger structural fixes such as #4628 (remove Arc<LogicalPlan>) are broader and somewhat out of scope here. - A lint to catch the pattern automatically. I don't know whether a suitable one exists, (could not find one if it already exists). Additional context Related open issues that I found: #19795 and #11316 Willingness to contribute: I plan to contribute to this feature independently. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
