Skip to content

Supports pushing down part of the filters when all filters cannot be pushed down#16412

Merged
highker merged 1 commit intoprestodb:masterfrom
wypb:improve-filter-pushdown
Jul 26, 2021
Merged

Supports pushing down part of the filters when all filters cannot be pushed down#16412
highker merged 1 commit intoprestodb:masterfrom
wypb:improve-filter-pushdown

Conversation

@wypb
Copy link
Contributor

@wypb wypb commented Jul 13, 2021 •
edited by highker
Loading

The corresponding issue is #16408

== RELEASE NOTES ==

JDBC Changes
* Support partial pushdown of JDBC filters.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 13, 2021 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

  • ✅ wyp (afe0cb0655d7e1dc00abc996fd67b56463a68022)

@xumingming
Copy link
Contributor

xumingming commented Jul 13, 2021

" Supports pushing down part of the filters when all filters cannot be pushed down in jdbc connector." -> "Support pushing down part of the filters when not all the filters can be push down in JDBC connectors."

@wypb
Copy link
Contributor Author

wypb commented Jul 16, 2021

Hi @sachdevs, can you help me to review this mr?

tdcmeehan
tdcmeehan approved these changes Jul 23, 2021
View reviewed changes
Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

Maybe @highker can take a look as well?

...ase-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/optimization/JdbcComputePushdown.java Outdated
Comment on lines 176 to 177
Copy link
Contributor

@tdcmeehan tdcmeehan Jul 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Please static import

Suggested change
List<String> sqlBodies = JdbcFilterToSqlTranslator.mergeSqlBodies(translatedExpressions);
List<ConstantExpression> variableBindings = JdbcFilterToSqlTranslator.mergeVariableBindings(translatedExpressions);
List<String> sqlBodies = mergeSqlBodies(translatedExpressions);
List<ConstantExpression> variableBindings = mergeVariableBindings(translatedExpressions);

@tdcmeehan tdcmeehan requested a review from highker July 23, 2021 03:08
@tdcmeehan
Copy link
Contributor

tdcmeehan commented Jul 23, 2021

Please reword commit message, perhaps:

Support partial pushdown of JDBC filters

highker
highker reviewed Jul 23, 2021
View reviewed changes
...ase-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/optimization/JdbcComputePushdown.java Outdated
Copy link

@highker highker Jul 23, 2021

Choose a reason for hiding this comment

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

else is redundant; remove

...ase-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/optimization/JdbcComputePushdown.java Outdated
Comment on lines 186 to 187
Copy link

@highker highker Jul 23, 2021

Choose a reason for hiding this comment

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

one param per line and leave the first line empty

func(
    var1,
    var2,
    ...)
{

...ase-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/optimization/JdbcComputePushdown.java Outdated
Comment on lines 140 to 142
Copy link

@highker highker Jul 23, 2021

Choose a reason for hiding this comment

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

move this above expressionOptimizer.optimize

...ase-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/optimization/JdbcComputePushdown.java Outdated
Comment on lines 151 to 153
Copy link

@highker highker Jul 23, 2021

Choose a reason for hiding this comment

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

pushable do not have to co-exist; we only use pushable.isEmpty; that holds true for translatedExpressions.isEmpty

...ase-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/optimization/JdbcComputePushdown.java Outdated
Copy link

@highker highker Jul 23, 2021

Choose a reason for hiding this comment

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

call this remainingExpressions to pair with translatedExpressions

...jdbc/src/test/java/com/facebook/presto/plugin/jdbc/optimization/TestJdbcComputePushdown.java Outdated
Copy link

@highker highker Jul 23, 2021

Choose a reason for hiding this comment

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

nit: no filter can be pushed down

...jdbc/src/test/java/com/facebook/presto/plugin/jdbc/optimization/TestJdbcComputePushdown.java Outdated
Copy link

@highker highker Jul 23, 2021

Choose a reason for hiding this comment

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

testJdbcComputePartialPushdown

...jdbc/src/test/java/com/facebook/presto/plugin/jdbc/optimization/TestJdbcComputePushdown.java Outdated
Copy link

@highker highker Jul 23, 2021

Choose a reason for hiding this comment

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

testJdbcComputePartialPushdownWithOr

...jdbc/src/test/java/com/facebook/presto/plugin/jdbc/optimization/TestJdbcComputePushdown.java Outdated
Copy link

@highker highker Jul 23, 2021

Choose a reason for hiding this comment

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

testJdbcComputeNoPushdown

...jdbc/src/test/java/com/facebook/presto/plugin/jdbc/optimization/TestJdbcComputePushdown.java Outdated
Copy link

@highker highker Jul 23, 2021

Choose a reason for hiding this comment

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

same use 3L instead of Long.valueOf(3)

@wypb
Copy link
Contributor Author

wypb commented Jul 23, 2021

Please reword commit message, perhaps:

Support partial pushdown of JDBC filters

Done.

@wypb
Copy link
Contributor Author

wypb commented Jul 23, 2021

Hi @highker thank you for your reply, I will modify the code according to the above review comments.

@wypb
Copy link
Contributor Author

wypb commented Jul 23, 2021

HI @highker I have made changes according to your comments, can you help me review the code again?

highker
highker approved these changes Jul 23, 2021
View reviewed changes
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

could squash the two commits into one?

@wypb
Copy link
Contributor Author

wypb commented Jul 26, 2021

could squash the two commits into one?

Done.

@highker highker merged commit 31266f5 into prestodb:master Jul 26, 2021
@NikhilCollooru NikhilCollooru mentioned this pull request Aug 3, 2021
Merged
This was referenced Aug 13, 2021
Merged
Closed
@wypb wypb deleted the improve-filter-pushdown branch August 13, 2021 06:46
@highker highker mentioned this pull request Oct 13, 2021
Merged
highker pushed a commit to highker/presto that referenced this pull request Oct 13, 2021
Add filter check for JDBC pushdown unit tests
52bec9f 
JDBC filter pushdown always needs to preserve the original filter for
correctness. This is to prevent regressions like prestodb#16412 where filters
are partially pushed down and there is no guard on Presto side.
highker pushed a commit that referenced this pull request Oct 13, 2021
Add filter check for JDBC pushdown unit tests
9bdc510 
JDBC filter pushdown always needs to preserve the original filter for
correctness. This is to prevent regressions like #16412 where filters
are partially pushed down and there is no guard on Presto side.
@slhmy slhmy mentioned this pull request Feb 9, 2022
Merged
@aaneja aaneja mentioned this pull request May 2, 2025
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@highker highker highker approved these changes

@tdcmeehan tdcmeehan tdcmeehan approved these changes

Assignees

@highker highker

@tdcmeehan tdcmeehan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wypb @xumingming @tdcmeehan @highker