Skip to content
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

Deprecate Expr::column_refs #11115

Merged
merged 1 commit into from
Jun 25, 2024
Merged

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jun 24, 2024

Which issue does this PR close?

Closes #10505

Follow on to #11067

Rationale for this change

Expr::column_refs is much more efficient than Expr::to_columns as it avoids copying strings. Thus we should
use it more in the codebase and encourage users to do the same

What changes are included in this PR?

Deprecate the function

Are these changes tested?

By existing CI

Are there any user-facing changes?

Function is deprcated

@github-actions github-actions bot added the logical-expr Logical plan and expressions label Jun 24, 2024
@alamb alamb force-pushed the alamb/deprecate_column_refs branch from eaca9d1 to 402c3db Compare June 24, 2024 23:11
@alamb alamb force-pushed the alamb/deprecate_column_refs branch from 402c3db to 47fba23 Compare June 24, 2024 23:13
@alamb alamb marked this pull request as ready for review June 24, 2024 23:13
@@ -1070,14 +1071,16 @@ impl LogicalPlanBuilder {
let left_key = l.into();
let right_key = r.into();

let left_using_columns = left_key.to_columns()?;
let mut left_using_columns = HashSet::new();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried several ways to avoid copying Columns here -- and I could not. Note that normalize_col_with_schemas_and_ambiguity_check takes left_key by move, so we can't simply call left_key.column_refs() as we can't move left_key when borrowed

@alamb alamb merged commit db64743 into apache:main Jun 25, 2024
23 checks passed
@alamb alamb deleted the alamb/deprecate_column_refs branch June 25, 2024 23:14
@alamb
Copy link
Contributor Author

alamb commented Jun 25, 2024

Thanks again @lewiszlw

findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logical-expr Logical plan and expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API to get all Column references in an Expr without cloning Columns
2 participants