-
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
Fix Pre Process Metadata Call when facing CTE Issue #22487
Fix Pre Process Metadata Call when facing CTE Issue #22487
Conversation
3de6f51
to
59400bd
Compare
Also is there a way to add tests? |
protected Void visitWith(With node, MetadataExtractorContext context) | ||
{ | ||
node.getQueries().forEach(query -> { | ||
context.addCTE(query.getName()); |
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.
Instead of calling it add addCTE, can we come up with some type agnostic name, such as skipProcessing(identifier)
@@ -186,7 +197,9 @@ protected Void visitTable(Table table, MetadataExtractorContext context) | |||
} | |||
|
|||
// This could be either tableName, view, or MView | |||
context.addTable(tableName); | |||
if (!context.tableExistsInCommonTableExpressionNames(new Identifier(tableName.getObjectName()))) { |
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.
Similar to below comment, this could be generalized by checking context.shouldSkipProcessing(identifier)?
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.
sounds good
+1, Lets add tests for with clause handling. |
Yea, I'm trying to figure out a valid test case which can fetch the corresponding |
59400bd
to
8f80ac1
Compare
@@ -125,17 +127,20 @@ private class MetadataExtractorContext | |||
{ | |||
private final Optional<MetadataExtractorContext> parent; | |||
private final Set<QualifiedObjectName> tableNames; | |||
private final Set<Identifier> tableNamesToSkipProcessing; // Table names, such as query with CTE, that we don't need to get the metadata info so we should skip sending request |
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.
Move the comment to top line
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.
done, thanks
8f80ac1
to
20e6e1a
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
thanks nikhil for helping merge it! |
@konjac-h Is there notable performance improvement to CTE selections? If yes, can you please provide a release note? Does the following sound good? Improve the performance of reading common table expressions (CTE). |
Description
When handling the query below with Common Table Expression (CTE)
If a QuerySpecification node has the Optional from, Visitor will visit the corresponding table (visitTable). Subsequently, MetadataExtractor will include the table in its list of tableNames and then call 1getTableColumnMetadata`.
It will lead to unnecessary invocations of getTableColumnMetadata and getTableHandle when a query has a Common Table Expression (CTE).
To fix this issue, we will need to determine if the
table_name
inside queryfrom <table_name>
is from the CTE expression. If yes, we need to exclude it without sending request togetTableColumnMetadata
Impact
Fixing the pre_process_meatdata calls. It will help us reduce a lot of unnecessary call to
getTableColumnMetadata
and latency.Test Plan
pre_process_metadata_calls
enabledContributor checklist