[FLINK-20767][table planner] Support filter push down on nested fields by venkata91 · Pull Request #23313 · apache/flink

venkata91 · 2023-08-29T01:13:15Z

What is the purpose of the change

Support filter push down on nested fields
For example:
SELECT * FROM users WHERE user.age > 18
In the above SQL, user.age > 18 filter can be pushed to table source to avoid scanning records that don't match the predicate. This is especially useful in analytics with columnar formats as well as in JDBC datasources.

Brief change log

Introduce a new ResolvedExpression called NestedFieldReferenceExpression to handle filters on nested fields
Changes to RexNodeToExpressionConverter and ExpressionConverter to convert RexFieldAccess <=> NestedFieldReferenceExpression and vice versa.

Verifying this change

Added unit tests in PushFilterIntoTableSourceScanRuleTest

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no)
The runtime per-record code paths (performance sensitive): (yes / no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no)
The S3 file system connector: (yes / no)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

flinkbot · 2023-08-29T01:18:24Z

CI report:

8924b93 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

venkata91 · 2023-08-30T23:54:42Z

@flinkbot run azure

venkata91 · 2023-09-07T17:48:50Z

@wuchong @swuferhong @JingsongLi @becketqin please review

venkata91 · 2023-09-07T17:49:19Z

Also tagging @slinkydeveloper

swuferhong

Hi, @venkata91 . Thanks for your contribution. The PR looks good in general. I left some comments.

swuferhong · 2023-09-13T04:21:35Z

+        util.verifyRelPlan(
+                "SELECT id FROM NestedTable WHERE `deepNestedWith.`.nested.```name` = 'foo'");


One small question, is this the standard way of adding "`"? I couldn't find the relevant documents.

This tests the case where the column name has backtick () in it and should be escaped as the whole nested field expression name itself has to be wrapped inside backticks. File formats like ORC etc requires the entire nested field to be with in backticks ()

becketqin

@venkata91 Thanks for the patch. LGTM overall. I agree with @swuferhong that we need to add some IT cases for E2E test.

venkata91 · 2023-09-14T07:32:48Z

@becketqin @swuferhong Addressed review comments mainly adding ITCases.

lsyldliu

@venkata91 Thanks for your contribution, please use git rebase instead of git merge. I can't get your all commits correctly when pull your pr to local.

venkata91 · 2023-09-18T23:34:33Z

@venkata91 Thanks for your contribution, please use git rebase instead of git merge. I can't get your all commits correctly when pull your pr to local.

@lsyldliu Do you want me to fix it?

venkata91 · 2023-09-18T23:40:55Z

@venkata91 Thanks for your contribution, please use git rebase instead of git merge. I can't get your all commits correctly when pull your pr to local.

@lsyldliu Do you want me to fix it?

I used to use rebase before. But with rebase the commit ids changes and therefore it is hard to only see the requested changes which is why I changed to merge instead of rebase. See here, started doing it after this comment.

- Implement nested fields filter push down 1. See if a new ResolvedExpression called NestedFieldReferenceExpression has to be implemented or not. If implemented, convert the NestedFieldReferenceExpression in to RexFieldAccess 2. Revisit ExpressionConverter to see if the FieldReferenceExpression has to be converted to a RexFieldAccess 3. Add more tests 4. Check other rules like PushFilterInCalcIntoTableSourceScanRule, PushPartitionIntoTableSourceScanRule

…y or map

… etc and all

venkata91 · 2023-09-18T23:46:39Z

@venkata91 Thanks for your contribution, please use git rebase instead of git merge. I can't get your all commits correctly when pull your pr to local.

@lsyldliu Do you want me to fix it?

nvm, fixed it as requested.

venkata91 · 2023-09-19T18:00:43Z

@swuferhong @becketqin @lsyldliu Gentle ping for reviews.

becketqin

LGTM, just one minor comment regarding the usage of Optional.

Ping @swuferhong for a review as well.

swuferhong

+1 LGTM

becketqin · 2023-09-26T23:13:50Z

Merged to master: 5be4688

venkata91 force-pushed the FLINK-20767 branch 2 times, most recently from 2583185 to bc31ebd Compare August 30, 2023 05:17

venkata91 marked this pull request as ready for review August 30, 2023 23:56

venkata91 force-pushed the FLINK-20767 branch 2 times, most recently from fe34258 to 74ccf82 Compare September 6, 2023 16:54

swuferhong reviewed Sep 13, 2023

View reviewed changes

becketqin reviewed Sep 13, 2023

View reviewed changes

Comment thread ...able-planner/src/main/scala/org/apache/flink/table/planner/plan/utils/RexNodeExtractor.scala

lsyldliu reviewed Sep 18, 2023

View reviewed changes

venkata91 added 10 commits September 18, 2023 16:45

changes based on FLIP to use NestedFieldReferenceExpression

3186606

Add more tests and don't push filters on nested fields inside an arra…

b2a7c61

…y or map

more changes

0229780

More changes based on the FLIP discussions

f5b7234

some changes

fbc35d2

Simplify the changes

0803862

Need to check whether this change for other cases like DeletePushDown…

c6825fd

… etc and all

Add fieldIndices to the NestedFieldReferenceExpression

428d884

Quote the nested field name

c26c780

venkata91 force-pushed the FLINK-20767 branch from 48b49b2 to dbd3cdf Compare September 18, 2023 23:46

becketqin approved these changes Sep 20, 2023

View reviewed changes

Comment thread ...e-planner/src/test/java/org/apache/flink/table/planner/factories/TestValuesTableFactory.java Outdated

Address review comments

8924b93

venkata91 force-pushed the FLINK-20767 branch from dbd3cdf to 8924b93 Compare September 22, 2023 20:01

swuferhong approved these changes Sep 25, 2023

View reviewed changes

becketqin merged commit 5be4688 into apache:master Sep 26, 2023

flinkbot added the component=TableSQL/Planner label Apr 4, 2024

		util.verifyRelPlan(
		"SELECT id FROM NestedTable WHERE `deepNestedWith.`.nested.```name` = 'foo'");

Conversation

venkata91 commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

flinkbot commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

venkata91 commented Aug 30, 2023

Uh oh!

venkata91 commented Sep 7, 2023

Uh oh!

venkata91 commented Sep 7, 2023

Uh oh!

swuferhong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swuferhong Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

venkata91 Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

becketqin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

venkata91 commented Sep 14, 2023

Uh oh!

lsyldliu left a comment

Choose a reason for hiding this comment

Uh oh!

venkata91 commented Sep 18, 2023

Uh oh!

venkata91 commented Sep 18, 2023

Uh oh!

venkata91 commented Sep 18, 2023

Uh oh!

venkata91 commented Sep 19, 2023

Uh oh!

becketqin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

swuferhong left a comment

Choose a reason for hiding this comment

Uh oh!

becketqin commented Sep 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

venkata91 commented Aug 29, 2023 •

edited

Loading

flinkbot commented Aug 29, 2023 •

edited

Loading