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)
There was a problem hiding this comment.
Yeah reassign_expr_columns returns a Result and we're filter mapping over the list of expressions so need to return an Option.
In this case it's okay to discard the result I think (per the comment)
| 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())?; |
|
@rkrishn7 are we good to merge this PR ? Or do you plan to address the comments prior to merge? |
|
Hey @alamb sorry I've been quite busy this week. I plan to address your comments either today or tomorrow! |
Ok, no problem! We can also do it as a follow on PR too if you prefer |
|
@alamb Updated w/ your suggestions! |
rkrishn7
force-pushed
the
chore/refactor-reassign-predicate-cols
branch
from
d478598 to
8b0fdcb
Compare
|
Thank you @rkrishn7 |
2 participants
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?
collect_columns_from_predicatehas been marked deprecated