Create QueryPreparer interface #18561

jainxrohit · 2022-10-25T17:57:07Z

Creating query preparer interface to support multiple analyzers. The basic idea is for engine to use analyzer agnostic concepts to support analyzer functionalities. In this change, we are leveraging query preparer interface to prepare the query. At this point, we are still relying on statement and BuiltInQueryPreparer from SQLQueryExecution state.

== RELEASE NOTES ==

General Changes
* Add ``CONTROL`` as a new ``QueryType``.  The CONTROL queryType represents statements of session control and transaction control types.

presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/AnalyzerClient.java

presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/BuiltInQueryPreparer.java

presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/PreparedQuery.java

presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/QueryPreparer.java

presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/AnalyzerClient.java

highker · 2022-10-27T02:14:33Z

Please cap the commit message to 72 characters per line.

highker · 2022-10-27T02:14:35Z

Please cap the commit message to 72 characters per line.

highker

I'm still reviewing; only skimmed through

presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/AnalyzerProvider.java

highker · 2022-10-27T04:52:19Z

presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/BuiltInQueryPreparer.java

+import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.joining;
+
+public class BuiltInQueryPreparer


presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/QueryPreparer.java

highker · 2022-10-27T04:54:41Z

presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/AnalyzerType.java

+ */
+package com.facebook.presto.sql.analyzer;
+
+public enum AnalyzerType


presto-common/src/main/java/com/facebook/presto/common/resourceGroups/QueryType.java

highker

overall LGTM % one big rollout concern

presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/QueryPreparer.java

presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/utils/StatementUtils.java

presto-common/src/main/java/com/facebook/presto/common/resourceGroups/QueryType.java

highker · 2022-10-28T03:50:03Z

presto-tests/src/test/resources/resource_groups_query_type_based_config.json

@@ -44,6 +44,12 @@
          "hardConcurrencyLimit": 3,
          "maxQueued": 4
        },
+        {
+          "name": "transaction",


[major] This reminds me that the rollout of this commit will cause pretty big production regression. We are now going to query queries like set session. Do we have a safe rollout strategy?

is it possible to have data definition and transaction share a resource group? (i mean this is for test, so doesn't matter much, but was wondering about prod -- does the categorization change need to mean different resource groups)

Good catch! We will make changes to the existing resource groups to account for this query type. We will make sure to account this for the next release.

presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/NativePreparedQuery.java

rschlussel · 2022-10-28T14:01:23Z

we log the query type in the query completed event, and also it can be used for resource group configurations, so we should add a release note that we've added another kind of query type/ changed the query type for some existing queries.

jainxrohit · 2022-10-28T14:42:44Z

we log the query type in the query completed event, and also it can be used for resource group configurations, so we should add a release note that we've added another kind of query type/ changed the query type for some existing queries.

I was planning to add a release note. However I am not sure about James comment, how it would cause production regression?

Using QueryType instead of statement class as key in the QueryExecutionFactory. We also added CONTROL as new QueryType to categorize statements of transaction control and session control types.

Adding a QueryPreparer interface, all analyzers should implement this interface to support query prepare functionality.

jainxrohit marked this pull request as ready for review October 25, 2022 18:01

jainxrohit requested a review from a team as a code owner October 25, 2022 18:01

jainxrohit requested review from presto-oss, rschlussel and highker October 25, 2022 18:01

jainxrohit force-pushed the rj_analyzer_refactoring branch from 2d52b10 to c7d6d5a Compare October 25, 2022 18:18

elharo reviewed Oct 25, 2022

View reviewed changes

jainxrohit force-pushed the rj_analyzer_refactoring branch 4 times, most recently from 9aaf607 to e28d182 Compare October 25, 2022 21:33

rschlussel reviewed Oct 26, 2022

View reviewed changes

presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/AnalyzerClient.java Outdated Show resolved Hide resolved

jainxrohit force-pushed the rj_analyzer_refactoring branch 2 times, most recently from 4e3946c to 67042a9 Compare October 27, 2022 00:14

jainxrohit requested a review from rschlussel October 27, 2022 00:16

jainxrohit force-pushed the rj_analyzer_refactoring branch 2 times, most recently from 7547b7f to 10cb683 Compare October 27, 2022 02:01

highker reviewed Oct 27, 2022

View reviewed changes

highker self-requested a review October 27, 2022 04:59

jainxrohit force-pushed the rj_analyzer_refactoring branch 6 times, most recently from 0ef64c8 to dc8334c Compare October 27, 2022 21:41

highker approved these changes Oct 28, 2022

View reviewed changes

highker self-assigned this Oct 28, 2022

jainxrohit force-pushed the rj_analyzer_refactoring branch from dc8334c to 68f75a6 Compare October 28, 2022 16:19

jainxrohit added 2 commits October 28, 2022 09:33

Use querytype as key in QueryExecutionFactory map

4d8a4ff

Using QueryType instead of statement class as key in the QueryExecutionFactory. We also added CONTROL as new QueryType to categorize statements of transaction control and session control types.

Rename QueryPreparer to BuiltInQueryPreparer

6e51433

jainxrohit force-pushed the rj_analyzer_refactoring branch from 68f75a6 to 2c90a57 Compare October 28, 2022 16:50

Add QueryPreparer interface

bf5a486

Adding a QueryPreparer interface, all analyzers should implement this interface to support query prepare functionality.

jainxrohit force-pushed the rj_analyzer_refactoring branch from 2c90a57 to bf5a486 Compare October 28, 2022 17:08

rschlussel approved these changes Oct 28, 2022

View reviewed changes

rschlussel merged commit e97afc8 into prestodb:master Oct 28, 2022

jainxrohit added the plugable-analyzer label Jan 10, 2023

wanglinsong mentioned this pull request Jan 12, 2023

Add release notes for 0.279 #18920

Merged

30 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create QueryPreparer interface #18561

Create QueryPreparer interface #18561

jainxrohit commented Oct 25, 2022 •

edited

Loading

highker commented Oct 27, 2022

highker commented Oct 27, 2022

highker left a comment

highker Oct 27, 2022

highker Oct 27, 2022

highker left a comment

highker Oct 28, 2022

rschlussel Oct 28, 2022

jainxrohit Oct 28, 2022

rschlussel commented Oct 28, 2022

jainxrohit commented Oct 28, 2022

Create QueryPreparer interface #18561

Create QueryPreparer interface #18561

Conversation

jainxrohit commented Oct 25, 2022 • edited Loading

highker commented Oct 27, 2022

highker commented Oct 27, 2022

highker left a comment

Choose a reason for hiding this comment

highker Oct 27, 2022

Choose a reason for hiding this comment

highker Oct 27, 2022

Choose a reason for hiding this comment

highker left a comment

Choose a reason for hiding this comment

highker Oct 28, 2022

Choose a reason for hiding this comment

rschlussel Oct 28, 2022

Choose a reason for hiding this comment

jainxrohit Oct 28, 2022

Choose a reason for hiding this comment

rschlussel commented Oct 28, 2022

jainxrohit commented Oct 28, 2022

jainxrohit commented Oct 25, 2022 •

edited

Loading