Skip to content
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.

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

Simplify expression transformer in Parquet predicate pushdown with ast::tree #17587

Open
wants to merge 1 commit into
base: branch-25.02
Choose a base branch
from

Conversation

mhaseeb123
Copy link
Member

@mhaseeb123 mhaseeb123 commented Dec 13, 2024

Description

This PR simplifies the StatsAST expression transformer in Parquet reader's predicate pushdown using ast::tree from (#17156).

This PR is a follow up to @bdice's comment at #17289 (comment). Similar changes for the BloomfilterAST expression converter have been incorporated in the PR #17289.

Related to #17164

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

Copy link

copy-pr-bot bot commented Dec 13, 2024

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@mhaseeb123 mhaseeb123 changed the title Simplify expression transformers in Parquet reader predicate pushdown with ast::tree 🚧 Simplify expression transformers in Parquet reader predicate pushdown with ast::tree Dec 13, 2024
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Dec 13, 2024
@@ -278,7 +277,6 @@ class stats_expression_converter : public ast::detail::expression_transformer {
"Statistics AST supports only left table");
CUDF_EXPECTS(expr.get_column_index() < _num_columns,
"Column index cannot be more than number of columns in the table");
_stats_expr = std::reference_wrapper<ast::expression const>(expr);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These don't need to be pushed to the ast::tree since we will be filtering with the new columns in the StatsAST table.

@karthikeyann
Copy link
Contributor

/ok to test

@mhaseeb123 mhaseeb123 added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Dec 13, 2024
@mhaseeb123 mhaseeb123 changed the title 🚧 Simplify expression transformers in Parquet reader predicate pushdown with ast::tree Simplify expression transformer in Parquet predicate pushdown with ast::tree Dec 13, 2024
@mhaseeb123 mhaseeb123 removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Dec 13, 2024
@mhaseeb123 mhaseeb123 requested a review from bdice December 13, 2024 22:56
@mhaseeb123 mhaseeb123 added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Dec 13, 2024
@mhaseeb123 mhaseeb123 marked this pull request as ready for review December 13, 2024 22:56
@mhaseeb123 mhaseeb123 requested a review from a team as a code owner December 13, 2024 22:56
@mhaseeb123 mhaseeb123 added 4 - Needs Review Waiting for reviewer to review or respond and removed 3 - Ready for Review Ready for review by team labels Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Needs Review Waiting for reviewer to review or respond cuIO cuIO issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants