Skip to content

Substrait support for propagating TableScan.filters to Substrait ReadRel.filter #14194

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.

Sign up for GitHub

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

Jump to bottom
Merged
alamb merged 12 commits into from
Mar 11, 2025
Merged

Substrait support for propagating TableScan.filters to Substrait ReadRel.filter #14194

alamb merged 12 commits into from
Mar 11, 2025

Conversation

@jamxia155
Copy link
Contributor

@jamxia155 jamxia155 commented Jan 19, 2025 •
edited
Loading

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 Substrait ReadRel.filter.
The conjunction of inexact filters in TableScan.filters is saved to Substrait ReadRel.filter.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

@github-actions github-actions bot added the substrait Changes to the substrait crate label Jan 19, 2025
@jamxia155
Copy link
Contributor Author

jamxia155 commented Jan 22, 2025

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.

@jonahgao
Copy link
Member

jonahgao commented Jan 22, 2025

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 and cargo test.

cargo clippy --all-targets --workspace --features avro,pyarrow -- -D warnings
cargo test --lib --tests --bins --features avro,json

jonahgao
jonahgao reviewed Jan 22, 2025
View reviewed changes
jonahgao
jonahgao reviewed Jan 22, 2025
View reviewed changes
@Blizzara
Copy link
Contributor

Blizzara commented Jan 22, 2025

would it be possible to add the matching support to consumer as well?

vbarua
vbarua reviewed Jan 22, 2025
View reviewed changes
@jamxia155 jamxia155 changed the title Substrait support for propagating TableScan.filters to Substrait ReadRel.best_effort_filter Substrait support for propagating TableScan.filters to Substrait ReadRel.filter Jan 22, 2025
@jamxia155 jamxia155 changed the title Substrait support for propagating TableScan.filters to Substrait ReadRel.filter Substrait support for propagating TableScan.filters to Substrait ReadRel.filter and ReadRel.best_effort_filter Jan 23, 2025
@jamxia155 jamxia155 force-pushed the jamxia_substrait_logical_producer_ReadRel_best_effort_filter branch from f09b057 to 6c6c74c Compare February 4, 2025 02:03
@jamxia155
Copy link
Contributor Author

jamxia155 commented Feb 4, 2025 •
edited
Loading

would it be possible to add the matching support to consumer as well?

Hi @Blizzara , finally got the time to add the matching support to consumer. Please share any comments if you get a chance. Thanks.

Blizzara
Blizzara reviewed Feb 13, 2025
View reviewed changes
Copy link
Contributor

@Blizzara Blizzara left a 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

@jamxia155 jamxia155 force-pushed the jamxia_substrait_logical_producer_ReadRel_best_effort_filter branch from 6c6c74c to 43717c4 Compare February 14, 2025 19:16
@alamb alamb mentioned this pull request Feb 16, 2025
Closed
@alamb alamb mentioned this pull request Feb 24, 2025
Closed
10 tasks
vbarua
vbarua reviewed Feb 26, 2025
View reviewed changes
jamxia155 added 11 commits March 1, 2025 08:41
@jamxia155
Propagate filter info from TableScan to ReadRel
2d5098d 
Propagate information in datafusion::logical_expr::TableScan.filters
to substrait::proto::ReadRel.best_effort_filter.
@jamxia155
Add test
3f8f262
@jamxia155
cargo fmt
203b2da
@jamxia155
Fix clippy error
204d300
@jamxia155
Use conjunction
867c763
@jamxia155
cargo fmt
0786208
@jamxia155
Use ReadRel.filter instead of best_effort_filter
9ff9d0a
@jamxia155
Check filter types in TableScan.filters
60635fb 
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
Propagate Substrait ReadRel filter to consumer
467aaaa
@jamxia155
Address PR comments
ab41a1b
@jamxia155
Propagate TableScan filters to ReadRel filter
f803557
@jamxia155 jamxia155 force-pushed the jamxia_substrait_logical_producer_ReadRel_best_effort_filter branch from 43717c4 to f803557 Compare March 1, 2025 16:42
@jamxia155 jamxia155 changed the title Substrait support for propagating TableScan.filters to Substrait ReadRel.filter and ReadRel.best_effort_filter Substrait support for propagating TableScan.filters to Substrait ReadRel.filter and ReadRel.filter Mar 2, 2025
@alamb alamb mentioned this pull request Mar 3, 2025
Closed
12 tasks
vbarua
vbarua approved these changes Mar 5, 2025
View reviewed changes
Copy link
Contributor

@vbarua vbarua left a 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 🙇

@jamxia155 jamxia155 changed the title Substrait support for propagating TableScan.filters to Substrait ReadRel.filter and ReadRel.filter Substrait support for propagating TableScan.filters to Substrait ReadRel.filter Mar 6, 2025
alamb
alamb approved these changes Mar 10, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a 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

@jamxia155
Copy link
Contributor Author

jamxia155 commented Mar 10, 2025

Hi @alamb, thanks for reviewing. Could you please approve the workflows?

@alamb alamb merged commit 04d823b into apache:main Mar 11, 2025
24 checks passed
@alamb
Copy link
Contributor

alamb commented Mar 11, 2025 •
edited
Loading

Thanks again @jamxia155 @vbarua @Blizzara and everyone else!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jonahgao jonahgao jonahgao left review comments

@alamb alamb alamb approved these changes

+4 more reviewers

@Blizzara Blizzara Blizzara left review comments

@westonpace westonpace westonpace left review comments

@nseekhao nseekhao nseekhao left review comments

@vbarua vbarua vbarua approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

substrait Changes to the substrait crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Substrait support for propagating TableScan.filters to Substrait ReadRel

7 participants

@jamxia155 @jonahgao @Blizzara @alamb @vbarua @westonpace @nseekhao