Add Expr::column_refs to find column references without copying #10948

alamb · 2024-06-17T09:59:47Z

Which issue does this PR close?

Rationale for this change

Code that uses Expr::to_columns or expr_to_columns copies the column name strings, often to simply check if a column is present. A non copying API would be more efficient, and now thanks to the improvements in the TreeNode API (from #10543, thanks @peter-toth!) it is straightforward to implement

What changes are included in this PR?

Add Expr::column_refs and Expr::add_column_refs
Updated some places in the code to use this new API. I will make a follow on ticket to deprecate the old apis and update code to use it
Deprecate exprlist_to_columns which is not used anywhere in the datafusion codebase

I plan to deprecate Expr::to_columns and expr_to_columns as follow on PRs

Are these changes tested?

Yes, by existing tests and new doc examples

Performance results show basically no improvement (maybe it gets faster for some of the queries with larger numbers of columns) but it may also be noise

Details

++ critcmp main less_to_column
group                                         less_to_column                         main
-----                                         --------------                         ----
logical_aggregate_with_join                   1.01  1013.4±56.98µs        ? ?/sec    1.00  1002.7±41.73µs        ? ?/sec
logical_plan_tpcds_all                        1.00    152.8±0.76ms        ? ?/sec    1.00    153.0±1.61ms        ? ?/sec
logical_plan_tpch_all                         1.00     17.0±0.21ms        ? ?/sec    1.00     16.9±0.19ms        ? ?/sec
logical_select_all_from_1000                  1.00     18.1±0.14ms        ? ?/sec    1.04     18.9±0.08ms        ? ?/sec
logical_select_one_from_700                   1.01    820.3±9.09µs        ? ?/sec    1.00    810.2±9.31µs        ? ?/sec
logical_trivial_join_high_numbered_columns    1.01   772.3±17.02µs        ? ?/sec    1.00    762.2±6.46µs        ? ?/sec
logical_trivial_join_low_numbered_columns     1.01   762.8±20.29µs        ? ?/sec    1.00   755.9±19.28µs        ? ?/sec
physical_plan_tpcds_all                       1.00   1226.8±5.71ms        ? ?/sec    1.00   1225.2±8.60ms        ? ?/sec
physical_plan_tpch_all                        1.00     83.6±1.50ms        ? ?/sec    1.00     83.8±1.04ms        ? ?/sec
physical_plan_tpch_q1                         1.04      4.6±0.27ms        ? ?/sec    1.00      4.5±0.04ms        ? ?/sec
physical_plan_tpch_q10                        1.00      4.0±0.04ms        ? ?/sec    1.00      4.0±0.04ms        ? ?/sec
physical_plan_tpch_q11                        1.01      3.6±0.05ms        ? ?/sec    1.00      3.5±0.04ms        ? ?/sec
physical_plan_tpch_q12                        1.01      2.7±0.03ms        ? ?/sec    1.00      2.7±0.02ms        ? ?/sec
physical_plan_tpch_q13                        1.00      2.0±0.02ms        ? ?/sec    1.00      2.0±0.02ms        ? ?/sec
physical_plan_tpch_q14                        1.01      2.4±0.02ms        ? ?/sec    1.00      2.4±0.03ms        ? ?/sec
physical_plan_tpch_q16                        1.01      3.4±0.05ms        ? ?/sec    1.00      3.4±0.03ms        ? ?/sec
physical_plan_tpch_q17                        1.00      3.3±0.04ms        ? ?/sec    1.00      3.3±0.04ms        ? ?/sec
physical_plan_tpch_q18                        1.00      3.7±0.11ms        ? ?/sec    1.00      3.7±0.03ms        ? ?/sec
physical_plan_tpch_q19                        1.00      5.4±0.07ms        ? ?/sec    1.00      5.4±0.05ms        ? ?/sec
physical_plan_tpch_q2                         1.00      7.3±0.07ms        ? ?/sec    1.00      7.2±0.08ms        ? ?/sec
physical_plan_tpch_q20                        1.00      4.2±0.07ms        ? ?/sec    1.00      4.2±0.06ms        ? ?/sec
physical_plan_tpch_q21                        1.00      5.8±0.07ms        ? ?/sec    1.00      5.8±0.07ms        ? ?/sec
physical_plan_tpch_q22                        1.01      3.1±0.03ms        ? ?/sec    1.00      3.1±0.03ms        ? ?/sec
physical_plan_tpch_q3                         1.00      2.9±0.03ms        ? ?/sec    1.00      2.9±0.02ms        ? ?/sec
physical_plan_tpch_q4                         1.01      2.2±0.02ms        ? ?/sec    1.00      2.2±0.01ms        ? ?/sec
physical_plan_tpch_q5                         1.01      4.1±0.05ms        ? ?/sec    1.00      4.1±0.03ms        ? ?/sec
physical_plan_tpch_q6                         1.01  1446.6±63.18µs        ? ?/sec    1.00   1434.8±8.77µs        ? ?/sec
physical_plan_tpch_q7                         1.01      5.2±0.06ms        ? ?/sec    1.00      5.2±0.06ms        ? ?/sec
physical_plan_tpch_q8                         1.01      6.7±0.08ms        ? ?/sec    1.00      6.6±0.10ms        ? ?/sec
physical_plan_tpch_q9                         1.00      5.1±0.06ms        ? ?/sec    1.00      5.1±0.04ms        ? ?/sec
physical_select_all_from_1000                 1.00     59.0±0.52ms        ? ?/sec    1.04     61.3±0.32ms        ? ?/sec
physical_select_one_from_700                  1.03      3.6±0.03ms        ? ?/sec    1.00      3.5±0.02ms        ? ?/sec

Are there any user-facing changes?

There is a new API

migrate some uses of to_column

alamb · 2024-06-17T10:00:56Z

datafusion/expr/src/expr.rs

+    /// assert!(refs.contains(&Column::new_unqualified("a")));
+    ///  assert!(refs.contains(&Column::new_unqualified("b")));
+    /// ```
+    pub fn column_refs(&self) -> HashSet<&Column> {


The implementation of this function is quite nice now compared to the expr_to_columns one: https://github.com/alamb/datafusion/blob/58d0c34d77c9a5202e62b9281cdbf1046abaa096/datafusion/expr/src/utils.rs#L264-L309

alamb · 2024-06-17T10:01:19Z

datafusion/expr/src/utils.rs

@@ -46,6 +46,7 @@ pub const COUNT_STAR_EXPANSION: ScalarValue = ScalarValue::Int64(Some(1));

 /// Recursively walk a list of expression trees, collecting the unique set of columns
 /// referenced in the expression
+#[deprecated(since = "40.0.0", note = "Expr::add_column_refs instead")]


this function is not used anywhere in the datafusion codebase

alamb · 2024-06-17T10:03:12Z

datafusion/optimizer/src/push_down_filter.rs

@@ -785,7 +782,7 @@ impl OptimizerRule for PushDownFilter {
                let mut keep_predicates = vec![];
                let mut push_predicates = vec![];
                for expr in predicates {
-                    let cols = expr.to_columns()?;
+                    let cols = expr.column_refs();


This is a pretty good example where there is no need to copy Columns simply to check if they are referenced.

peter-toth · 2024-06-17T11:06:28Z

This PR looks really nice, let me know when it is ready for review.

alamb · 2024-06-17T16:30:12Z

I think this is now ready for review. I ran benchmarks and they show some slight improvement

datafusion/optimizer/src/analyzer/subquery.rs

peter-toth

LGTM, I have only a minor suggestion.

comphead

lgtm thanks @alamb

alamb · 2024-06-20T17:00:16Z

Thanks @comphead and @peter-toth

…he#10948) * Add Expr::column_refs to find column references without copying migrate some uses of to_column * Simplify condition

Add Expr::column_refs to find column references without copying

0bf8ccb

migrate some uses of to_column

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules labels Jun 17, 2024

alamb mentioned this pull request Jun 17, 2024

API to get all Column references in an Expr without cloning Columns #10505

Closed

alamb commented Jun 17, 2024

View reviewed changes

alamb marked this pull request as ready for review June 17, 2024 10:52

alamb marked this pull request as draft June 17, 2024 10:52

alamb marked this pull request as ready for review June 17, 2024 16:29

peter-toth reviewed Jun 17, 2024

View reviewed changes

datafusion/optimizer/src/analyzer/subquery.rs Outdated Show resolved Hide resolved

peter-toth approved these changes Jun 17, 2024

View reviewed changes

alamb added 2 commits June 18, 2024 09:18

Merge remote-tracking branch 'apache/main' into alamb/less_to_column

09dba99

Simplify condition

cefb7bd

comphead approved these changes Jun 19, 2024

View reviewed changes

Merge remote-tracking branch 'apache/main' into alamb/less_to_column

6857618

alamb merged commit 98373ab into apache:main Jun 22, 2024
23 checks passed

alamb deleted the alamb/less_to_column branch June 22, 2024 12:51

alamb mentioned this pull request Jun 22, 2024

Migrate more code from Expr::to_columns to Expr::column_refs #11067

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Expr::column_refs to find column references without copying #10948

Add Expr::column_refs to find column references without copying #10948

alamb commented Jun 17, 2024 •

edited

Loading

alamb Jun 17, 2024

alamb Jun 17, 2024

alamb Jun 17, 2024

peter-toth commented Jun 17, 2024

alamb commented Jun 17, 2024

peter-toth left a comment

comphead left a comment

alamb commented Jun 20, 2024

Add Expr::column_refs to find column references without copying #10948

Add Expr::column_refs to find column references without copying #10948

Conversation

alamb commented Jun 17, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb Jun 17, 2024

Choose a reason for hiding this comment

alamb Jun 17, 2024

Choose a reason for hiding this comment

alamb Jun 17, 2024

Choose a reason for hiding this comment

peter-toth commented Jun 17, 2024

alamb commented Jun 17, 2024

peter-toth left a comment

Choose a reason for hiding this comment

comphead left a comment

Choose a reason for hiding this comment

alamb commented Jun 20, 2024

alamb commented Jun 17, 2024 •

edited

Loading