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? |
jamxia155
changed the title
jamxia155
changed the title
jamxia155
force-pushed
the
jamxia_substrait_logical_producer_ReadRel_best_effort_filter
branch
from
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. |
Blizzara
left a comment
Blizzara
left a comment
There was a problem hiding this comment.
Thanks for adding the consumer part! I left some nits, but overall the logic seems sane to me now
jamxia155
force-pushed
the
jamxia_substrait_logical_producer_ReadRel_best_effort_filter
branch
from
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
jamxia155
force-pushed
the
jamxia_substrait_logical_producer_ReadRel_best_effort_filter
branch
from
43717c4 to
f803557
Compare
jamxia155
changed the title
vbarua
left a comment
vbarua
left a comment
There was a problem hiding this comment.
This looks good to me from a Substrait perspective ✨
Thanks for following through on this @jamxia155 🙇
jamxia155
changed the title
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
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! |
7 participants
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.filtersis saved to SubstraitReadRel.filter.The conjunction of inexact filters in
TableScan.filtersis saved to SubstraitReadRel.filter.Are these changes tested?
Yes.
Are there any user-facing changes?
No.