chore: refactor usage of reassign_predicate_columns
#17703
Conversation
github-actions
bot
added
physical-expr
|
|
||
| /// Return the equals Column-Pairs and Non-equals Column-Pairs | ||
| pub fn collect_columns_from_predicate( | ||
| fn collect_columns_from_predicate( |
There was a problem hiding this comment.
I'm just realizing this was pub and not pub(crate) so technically this is an API change
There was a problem hiding this comment.
maybe you can leave a #deprecated version of the old name (that just calles the new name) as described in https://datafusion.apache.org/contributor-guide/api-health.html#deprecation-guidelines
|
|
||
| /// Return the equals Column-Pairs and Non-equals Column-Pairs | ||
| pub fn collect_columns_from_predicate( | ||
| fn collect_columns_from_predicate( |
There was a problem hiding this comment.
maybe you can leave a #deprecated version of the old name (that just calles the new name) as described in https://datafusion.apache.org/contributor-guide/api-health.html#deprecation-guidelines
| // Ignore any binary expressions that reference non-existent columns in the current schema | ||
| // (e.g. due to unnecessary projections being removed) | ||
| reassign_expr_columns(Arc::clone(expr), schema) | ||
| .ok() |
There was a problem hiding this comment.
is this ok actually required? Does anything fail if we remove it?
It seems suspicious to me (though I see it is how the old code works too)
| let index = match schema.index_of(column.name()) { | ||
| Ok(idx) => idx, | ||
| Err(_) if ignore_not_found => usize::MAX, | ||
| Err(e) => return Err(e.into()), | ||
| }; |
There was a problem hiding this comment.
You could further simplify this to
| let index = match schema.index_of(column.name()) { | |
| Ok(idx) => idx, | |
| Err(_) if ignore_not_found => usize::MAX, | |
| Err(e) => return Err(e.into()), | |
| }; | |
| let index = match schema.index_of(column.name())?; |
Which issue does this PR close?
reassign_predicate_columns#17581What changes are included in this PR?
reassign_predicate_columnstoreassign_expr_columnsAre these changes tested?
No, there should be no changes in functionality.
Are there any user-facing changes?
N/A