-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Add materialized view query optimization utility #16253
Conversation
...-main/src/test/java/com/facebook/presto/sql/analyzer/TestMaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
...-main/src/test/java/com/facebook/presto/sql/analyzer/TestMaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
...-main/src/test/java/com/facebook/presto/sql/analyzer/TestMaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
cf9b6b4
to
d9d6470
Compare
@jzzhaofb the release note needs to have triple ` at the start and end.
|
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
be2d0d6
to
cf4bc7f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall in pretty good shape. Good work!
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still can't quickly get what types of base query cannot be optimized. Can you add the invalid base query types to TestMaterializedViewQueryOptimizer? A complete unit test suite should include invalid test cases.
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
@gggrace14 Julian and I discussed about it, the idea for this class is not to cover all non supported cases. We will add MV query validation logic in the candidate extractor. At that time we will add extended set of tests for not supported cases. |
cf4bc7f
to
a49fac7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one nit requested.
Awesome work!
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
a49fac7
to
9e9a3bd
Compare
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
...-main/src/test/java/com/facebook/presto/sql/analyzer/TestMaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
...-main/src/test/java/com/facebook/presto/sql/analyzer/TestMaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
Adding @yuanzhanhku to the review as he has a lot of insight how to check if a MV query is a proper subquery of a select query |
we need to properly check a scan-filter-project-agg query containment problem. To decide if we can replace a subquery in the given provided select query, we will need to at least satisfy the following conditions:
|
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Show resolved
Hide resolved
...ain/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewInformationExtractor.java
Show resolved
Hide resolved
...ain/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewInformationExtractor.java
Outdated
Show resolved
Hide resolved
...ain/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewInformationExtractor.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
...ain/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewInformationExtractor.java
Outdated
Show resolved
Hide resolved
The PR has lots of comments, lets clean those and keep the relevant ones only. |
db1ff90
to
fcab950
Compare
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
...-main/src/test/java/com/facebook/presto/sql/analyzer/TestMaterializedViewQueryOptimizer.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
...-main/src/test/java/com/facebook/presto/sql/analyzer/TestMaterializedViewQueryOptimizer.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
...-main/src/test/java/com/facebook/presto/sql/analyzer/TestMaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
...ain/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewInformationExtractor.java
Outdated
Show resolved
Hide resolved
fcab950
to
2f87a63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few minor changes requested.
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
...ain/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewInformationExtractor.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
...ain/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewInformationExtractor.java
Outdated
Show resolved
Hide resolved
2f87a63
to
d4c949d
Compare
@jzzhaofb Lets change the commit and PR to following Title: Add materialized view query optimization utility Adding materialized view optimizer utility to optimize eligible queries if supported by a materialized view. The utility is also responsible for validating the original query against the supported formats. Currently, this utility is very restrictive and supports some simple query formats. It does not support complex queries (Join, Union, subquery, etc.). |
d4c949d
to
cc7deb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@highker please have a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly nits
...ain/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewInformationExtractor.java
Outdated
Show resolved
Hide resolved
...ain/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewInformationExtractor.java
Outdated
Show resolved
Hide resolved
...ain/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewInformationExtractor.java
Outdated
Show resolved
Hide resolved
...ain/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewInformationExtractor.java
Outdated
Show resolved
Hide resolved
...ain/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewInformationExtractor.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
...ain/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewInformationExtractor.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/MaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
...-main/src/test/java/com/facebook/presto/sql/analyzer/TestMaterializedViewQueryOptimizer.java
Outdated
Show resolved
Hide resolved
Adding materialized view optimizer utility to optimize eligible queries if supported by a materialized view. The utility is also responsible for validating the original query against the supported formats. Currently, this utility is very restrictive and supports some simple query formats. It does not support complex queries (Join, Union, subquery, etc.).
cc7deb8
to
aa768f5
Compare
Adding materialized view optimizer utility to optimize eligible queries if supported by a materialized view. The utility is also responsible for validating the original query against the supported formats.
Currently, this utility is very restrictive and supports some simple query formats. It does not support complex queries (Join, Union, subquery, etc.).
== NO RELEASE NOTE ==