-
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?
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 ==