-
Notifications
You must be signed in to change notification settings - Fork 5.5k
feat: Add queryType to ConnectorSession to send query meta information #26474
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
base: master
Are you sure you want to change the base?
Conversation
Reviewer's GuideIntroduce QueryType support in ConnectorSession and implement it in session classes to allow SELECT queries to trigger ensureConsistentRead in Metastore API calls. Sequence diagram for SELECT query triggering ensureConsistentRead in Metastore APIsequenceDiagram
actor User
participant PrestoCoordinator
participant ConnectorSession
participant MetastoreAPI
User->>PrestoCoordinator: Submit SELECT query
PrestoCoordinator->>ConnectorSession: getQueryType()
ConnectorSession-->>PrestoCoordinator: QueryType.SELECT
PrestoCoordinator->>MetastoreAPI: Read with ensureConsistentRead=true
MetastoreAPI-->>PrestoCoordinator: Return results
PrestoCoordinator-->>User: Return query results
Class diagram for updated ConnectorSession and session implementationsclassDiagram
class ConnectorSession {
+getRuntimeStats()
+getQueryType() Optional<QueryType>
+forConnectorId(ConnectorId)
}
class FullConnectorSession {
+getRuntimeStats()
+getQueryType() Optional<QueryType>
+forConnectorId(ConnectorId)
}
class TestingConnectorSession {
+getRuntimeStats()
+getQueryType() Optional<QueryType>
+forConnectorId(ConnectorId)
}
ConnectorSession <|.. FullConnectorSession
ConnectorSession <|.. TestingConnectorSession
class QueryType {
<<enum>>
SELECT
// ... other query types
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
|
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.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `presto-spi/src/main/java/com/facebook/presto/spi/ConnectorSession.java:71` </location>
<code_context>
RuntimeStats getRuntimeStats();
+ Optional<QueryType> getQueryType();
+
/**
</code_context>
<issue_to_address>
**suggestion:** Documenting the semantics of getQueryType() could help future maintainers.
Clarifying the expected values and conditions for returning Optional.empty would make the method's behavior more transparent.
```suggestion
/**
* Returns the {@link QueryType} for the current session, if available.
* <p>
* The returned {@link Optional} will contain a value if the query type is known
* (e.g., for queries submitted via the main Presto engine). If the query type cannot
* be determined (such as for internal or system queries), {@link Optional#empty()} is returned.
*
* @return an {@link Optional} containing the {@link QueryType} if available, otherwise {@link Optional#empty()}
*/
Optional<QueryType> getQueryType();
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| RuntimeStats getRuntimeStats(); | ||
|
|
||
| Optional<QueryType> getQueryType(); |
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.
suggestion: Documenting the semantics of getQueryType() could help future maintainers.
Clarifying the expected values and conditions for returning Optional.empty would make the method's behavior more transparent.
| Optional<QueryType> getQueryType(); | |
| /** | |
| * Returns the {@link QueryType} for the current session, if available. | |
| * <p> | |
| * The returned {@link Optional} will contain a value if the query type is known | |
| * (e.g., for queries submitted via the main Presto engine). If the query type cannot | |
| * be determined (such as for internal or system queries), {@link Optional#empty()} is returned. | |
| * | |
| * @return an {@link Optional} containing the {@link QueryType} if available, otherwise {@link Optional#empty()} | |
| */ | |
| Optional<QueryType> getQueryType(); |
…ELECT queries. (prestodb#26474) Summary: X-link: facebookexternal/presto-facebook#3436 feat: Send "ensureConsistentRead" to Metastore API for all SELECT queries. BREAKING CHANGE: Default metastore replica to be read from for Select queries will be the read replica, and re-after-write users will have to set "prism.force_metastore_primary_enabled" session property to "true" Reviewed By: konjac-h Differential Revision: D84520611
cffbc4b to
5d29ff4
Compare
…ELECT queries (prestodb#26474) Summary: X-link: facebookexternal/presto-facebook#3436 Feat: Send "ensureConsistentRead" to Metastore API for all SELECT queries BREAKING CHANGE: Default metastore replica to be read from for Select queries will be the read replica, and re-after-write users will have to set "prism.force_metastore_primary_enabled" session property to "true" Reviewed By: konjac-h Differential Revision: D84520611
5d29ff4 to
8b2d69d
Compare
…eries (prestodb#26474) Summary: X-link: facebookexternal/presto-facebook#3436 feat: Send "ensureConsistentRead" to Metastore API for all SELECT queries BREAKING CHANGE: Default metastore replica to be read from for Select queries will be the read replica, and re-after-write users will have to set "prism.force_metastore_primary_enabled" session property to "true" Differential Revision: D84520611 Pulled By: adheer-araokar
8b2d69d to
1ec1211
Compare
…eries (prestodb#26474) Summary: X-link: facebookexternal/presto-facebook#3436 X-link: facebookexternal/presto-facebook#3436 feat: Send "ensureConsistentRead" to Metastore API for all SELECT queries BREAKING CHANGE: Default metastore replica to be read from for Select queries will be the read replica, and re-after-write users will have to set "prism.force_metastore_primary_enabled" session property to "true" Differential Revision: D84520611 Pulled By: adheer-araokar
1ec1211 to
7a602a5
Compare
|
The change actually is not a breaking change and simply adds the queryType to the connector session object to add meta information about the query. I have now fixed the title and the summary.
This session property is not relevant here either. Changed the summary. |
7a602a5 to
43c760c
Compare
…eries (prestodb#26474) Summary: X-link: facebookexternal/presto-facebook#3436 X-link: facebookexternal/presto-facebook#3436 feat: Send "ensureConsistentRead" to Metastore API for all SELECT queries BREAKING CHANGE: Default metastore replica to be read from for Select queries will be the read replica, and re-after-write users will have to set "prism.force_metastore_primary_enabled" session property to "true" Differential Revision: D84520611 Pulled By: adheer-araokar
…eries (prestodb#26474) Summary: X-link: facebookexternal/presto-facebook#3436 X-link: facebookexternal/presto-facebook#3436 feat: Send "ensureConsistentRead" to Metastore API for all SELECT queries BREAKING CHANGE: Default metastore replica to be read from for Select queries will be the read replica, and re-after-write users will have to set "prism.force_metastore_primary_enabled" session property to "true" Differential Revision: D84520611 Pulled By: adheer-araokar
43c760c to
937161e
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.
Where this new API getQueryType() been used?
|
@adheer-araokar : Thanks for this info. Can you add some test that checks the value returned by queryType for different cases ? |
937161e to
52c5add
Compare
…eries (prestodb#26474) Summary: X-link: facebookexternal/presto-facebook#3436 X-link: facebookexternal/presto-facebook#3436 feat: Send "ensureConsistentRead" to Metastore API for all SELECT queries BREAKING CHANGE: Default metastore replica to be read from for Select queries will be the read replica, and re-after-write users will have to set "prism.force_metastore_primary_enabled" session property to "true" Reviewed By: singcha Test Plan: `ensure_consistent_read` is set to false for Select and Explain queries, and to true for all other type of queries ``` select * from test_adheer_delete limit 100 ``` ``` headers: [{thrift_metastore_client_info={ "project" : "presto", "max_num_partitions_to_be_cached" : "1000", **"ensure_consistent_read" : "false",** "dw.jobId" : "20251027_045135_00005_ig79c", "dw.owner" : "prestoui", "dw.pipeline" : "presto-js-client" }}] ``` ``` CREATE TABLE test_adheer_delete (col1 bigint, col2 bigint) WITH (oncall = 'presto_batch', partitioned_by = ARRAY['col2']) ``` ``` headers: [{thrift_metastore_client_info={ "project" : "presto", "max_num_partitions_to_be_cached" : "1000", **"ensure_consistent_read" : "true",** "dw.jobId" : "20251027_043305_00004_ig79c", "dw.owner" : "prestoui", "dw.pipeline" : "presto-js-client" }}] ``` built presto version `0.296-20251028.203517-312` deployed on test cluster `atn1_adhoctest_t6_ec1` set the session property `SET SESSION prism.force_metastore_primary_enabled=true;` ran the query `select * from presto_query_statistics limit 1;` with the query id `20251028_234254_00028_yqy68` validated the headers with the query `select headers, * from hive_metsatore_logger_logs_inc_archive where ds = '2025-10-28' and dw_environment = 'atn1_adhoctest_t6_ec1' and dw_job_id = '20251028_234254_00028_yqy68'` {F1983081638} can see `ensure_consistent_read: true` ran the same query but without setting the session property, and validated the headers {F1983082149} can see `ensure_consistent_read: false` Differential Revision: D84520611 Pulled By: adheer-araokar
…eries (prestodb#26474) Summary: X-link: facebookexternal/presto-facebook#3436 X-link: facebookexternal/presto-facebook#3436 feat: Send "ensureConsistentRead" to Metastore API for all SELECT queries BREAKING CHANGE: Default metastore replica to be read from for Select queries will be the read replica, and re-after-write users will have to set "prism.force_metastore_primary_enabled" session property to "true" Reviewed By: singcha Test Plan: `ensure_consistent_read` is set to false for Select and Explain queries, and to true for all other type of queries ``` select * from test_adheer_delete limit 100 ``` ``` headers: [{thrift_metastore_client_info={ "project" : "presto", "max_num_partitions_to_be_cached" : "1000", **"ensure_consistent_read" : "false",** "dw.jobId" : "20251027_045135_00005_ig79c", "dw.owner" : "prestoui", "dw.pipeline" : "presto-js-client" }}] ``` ``` CREATE TABLE test_adheer_delete (col1 bigint, col2 bigint) WITH (oncall = 'presto_batch', partitioned_by = ARRAY['col2']) ``` ``` headers: [{thrift_metastore_client_info={ "project" : "presto", "max_num_partitions_to_be_cached" : "1000", **"ensure_consistent_read" : "true",** "dw.jobId" : "20251027_043305_00004_ig79c", "dw.owner" : "prestoui", "dw.pipeline" : "presto-js-client" }}] ``` built presto version `0.296-20251028.203517-312` deployed on test cluster `atn1_adhoctest_t6_ec1` set the session property `SET SESSION prism.force_metastore_primary_enabled=true;` ran the query `select * from presto_query_statistics limit 1;` with the query id `20251028_234254_00028_yqy68` validated the headers with the query `select headers, * from hive_metsatore_logger_logs_inc_archive where ds = '2025-10-28' and dw_environment = 'atn1_adhoctest_t6_ec1' and dw_job_id = '20251028_234254_00028_yqy68'` {F1983081638} can see `ensure_consistent_read: true` ran the same query but without setting the session property, and validated the headers {F1983082149} can see `ensure_consistent_read: false` Differential Revision: D84520611 Pulled By: adheer-araokar
52c5add to
bd83e99
Compare
…eries (prestodb#26474) Summary: X-link: facebookexternal/presto-facebook#3436 X-link: facebookexternal/presto-facebook#3436 feat: Send "ensureConsistentRead" to Metastore API for all SELECT queries BREAKING CHANGE: Default metastore replica to be read from for Select queries will be the read replica, and re-after-write users will have to set "prism.force_metastore_primary_enabled" session property to "true" Reviewed By: singcha Test Plan: `ensure_consistent_read` is set to false for Select and Explain queries, and to true for all other type of queries ``` select * from test_adheer_delete limit 100 ``` ``` headers: [{thrift_metastore_client_info={ "project" : "presto", "max_num_partitions_to_be_cached" : "1000", **"ensure_consistent_read" : "false",** "dw.jobId" : "20251027_045135_00005_ig79c", "dw.owner" : "prestoui", "dw.pipeline" : "presto-js-client" }}] ``` ``` CREATE TABLE test_adheer_delete (col1 bigint, col2 bigint) WITH (oncall = 'presto_batch', partitioned_by = ARRAY['col2']) ``` ``` headers: [{thrift_metastore_client_info={ "project" : "presto", "max_num_partitions_to_be_cached" : "1000", **"ensure_consistent_read" : "true",** "dw.jobId" : "20251027_043305_00004_ig79c", "dw.owner" : "prestoui", "dw.pipeline" : "presto-js-client" }}] ``` built presto version `0.296-20251028.203517-312` deployed on test cluster `atn1_adhoctest_t6_ec1` set the session property `SET SESSION prism.force_metastore_primary_enabled=true;` ran the query `select * from presto_query_statistics limit 1;` with the query id `20251028_234254_00028_yqy68` validated the headers with the query `select headers, * from hive_metsatore_logger_logs_inc_archive where ds = '2025-10-28' and dw_environment = 'atn1_adhoctest_t6_ec1' and dw_job_id = '20251028_234254_00028_yqy68'` {F1983081638} can see `ensure_consistent_read: true` ran the same query but without setting the session property, and validated the headers {F1983082149} can see `ensure_consistent_read: false` Differential Revision: D84520611 Pulled By: adheer-araokar
bd83e99 to
0a1c34b
Compare
feat: Add queryType to ConnectorSession to add query meta information to the session
Release Notes
Please follow release notes guidelines and fill in the release notes below.
== NO RELEASE NOTE ==