-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Substrait support for propagating TableScan.filters to Substrait ReadRel.filter #14194
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Substrait support for propagating TableScan.filters to Substrait ReadRel.filter #14194
Conversation
Hi @jonahgao , I've fixed the previous workflow failure. Would you be able to approve the latest workflow please? Or would you recommend a set of checks I should do offline first? Thanks. |
Done. The most commonly used offline checks are cargo clippy --all-targets --workspace --features avro,pyarrow -- -D warnings
cargo test --lib --tests --bins --features avro,json |
would it be possible to add the matching support to consumer as well? |
f09b057
to
6c6c74c
Compare
Hi @Blizzara , finally got the time to add the matching support to consumer. Please share any comments if you get a chance. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the consumer part! I left some nits, but overall the logic seems sane to me now
6c6c74c
to
43717c4
Compare
Propagate information in datafusion::logical_expr::TableScan.filters to substrait::proto::ReadRel.best_effort_filter.
Use TableScan.source.supports_filters_pushdown() to determine if each filter in TableScan.filters should be included in ReadRel.filter or ReadRel.best_effort_filter
43717c4
to
f803557
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me from a Substrait perspective ✨
Thanks for following through on this @jamxia155 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jamxia155 and @vbarua
I am sorry I missed this -- please do feel free to mention me or one of the other committers if a PR is ready for review
…cal_producer_ReadRel_best_effort_filter
Hi @alamb, thanks for reviewing. Could you please approve the workflows? |
Thanks again @jamxia155 @vbarua @Blizzara and everyone else! |
Which issue does this PR close?
Closes #14193.
Rationale for this change
Substrait producer currently does not propagate TableScan.filters into Substrait ReadRel. This results in loss of filter predicate pushdown information for Substrait consumers.
What changes are included in this PR?
The conjunction of exact filters in
TableScan.filters
is saved to SubstraitReadRel.filter
.The conjunction of inexact filters in
TableScan.filters
is saved to SubstraitReadRel.filter
.Are these changes tested?
Yes.
Are there any user-facing changes?
No.