-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Avoid redundant metastore calls for non-system Iceberg tables #8689
Avoid redundant metastore calls for non-system Iceberg tables #8689
Conversation
When table name doesn't look like a system table, we can exit early.
894ebd4
to
bbe7fd4
Compare
@@ -83,7 +83,7 @@ public void testCreateTable() | |||
ImmutableMultiset.builder() | |||
.add(CREATE_TABLE) | |||
.add(GET_DATABASE) | |||
.addCopies(GET_TABLE, 2) | |||
.add(GET_TABLE) |
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.
In another PR i was suggested to use only addCopies
and replace add
with addCopies(...,1)
.
I see you did the opposite here ;) For me add
is fine but just leaving this comment here to be consistent
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 think it was about consistency of neighbouring lines.
i do hope we will bring those counts to 1 (if by nothing else, then with some metastore cache)
and then we will have add(element)
consistently.
I do not know whether it that other PR count of 1 is the expected value, so maybe there is a difference.
@@ -303,6 +303,9 @@ public IcebergTableHandle getTableHandle(ConnectorSession session, SchemaTableNa | |||
private Optional<SystemTable> getRawSystemTable(ConnectorSession session, SchemaTableName tableName) | |||
{ | |||
IcebergTableName name = IcebergTableName.from(tableName.getTableName()); | |||
if (name.getTableType() == DATA) { | |||
return Optional.empty(); |
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.
We can also remove the metastore call below if we translate the not found exception into empty.
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.
Right, will handle in a follow up, since this also requires more test coverage (#8690)
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.
Addressed in #8692
When table name doesn't look like a system table, we can exit early.
For #8675