Skip to content
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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 28, 2021
Merged

Avoid redundant metastore calls for non-system Iceberg tables #8689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,9 @@ public Optional<SystemTable> getSystemTable(ConnectorSession session, SchemaTabl
private Optional<SystemTable> getRawSystemTable(ConnectorSession session, SchemaTableName tableName)
{
IcebergTableName name = IcebergTableName.from(tableName.getTableName());
if (name.getTableType() == DATA) {
return Optional.empty();
Copy link
Member

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.

Copy link
Member Author

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #8692

}

Optional<Table> hiveTable = metastore.getTable(new HiveIdentity(session), tableName.getSchemaName(), name.getTableName());
if (hiveTable.isEmpty() || !isIcebergTable(hiveTable.get())) {
Expand All @@ -314,6 +317,7 @@ private Optional<SystemTable> getRawSystemTable(ConnectorSession session, Schema
SchemaTableName systemTableName = new SchemaTableName(tableName.getSchemaName(), name.getTableNameWithType());
switch (name.getTableType()) {
case DATA:
// Handled above.
break;
case HISTORY:
if (name.getSnapshotId().isPresent()) {
Expand Down
16 changes: 8 additions & 8 deletions ...o-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMetastoreAccessOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public void testCreateTable()
ImmutableMultiset.builder()
.add(CREATE_TABLE)
.add(GET_DATABASE)
.addCopies(GET_TABLE, 2)
.add(GET_TABLE)
Copy link
Member

@homar homar Jul 28, 2021 •
edited
Loading

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

Copy link
Member Author

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.

.build());
}

Expand All @@ -94,7 +94,7 @@ public void testCreateTableAsSelect()
ImmutableMultiset.builder()
.add(GET_DATABASE)
.add(CREATE_TABLE)
.addCopies(GET_TABLE, 2)
.add(GET_TABLE)
.build());
}

Expand All @@ -105,7 +105,7 @@ public void testSelect()

assertMetastoreInvocations("SELECT * FROM test_select_from",
ImmutableMultiset.builder()
.addCopies(GET_TABLE, 12)
.addCopies(GET_TABLE, 8)
.build());
}

Expand All @@ -116,7 +116,7 @@ public void testSelectWithFilter()

assertMetastoreInvocations("SELECT * FROM test_select_from_where WHERE age = 2",
ImmutableMultiset.builder()
.addCopies(GET_TABLE, 12)
.addCopies(GET_TABLE, 8)
.build());
}

Expand All @@ -128,7 +128,7 @@ public void testJoin()

assertMetastoreInvocations("SELECT name, age FROM test_join_t1 JOIN test_join_t2 ON test_join_t2.id = test_join_t1.id",
ImmutableMultiset.builder()
.addCopies(GET_TABLE, 24)
.addCopies(GET_TABLE, 16)
.build());
}

Expand All @@ -139,7 +139,7 @@ public void testExplainSelect()

assertMetastoreInvocations("EXPLAIN SELECT * FROM test_explain",
ImmutableMultiset.builder()
.addCopies(GET_TABLE, 11)
.addCopies(GET_TABLE, 7)
.build());
}

Expand All @@ -150,7 +150,7 @@ public void testShowStatsForTable()

assertMetastoreInvocations("SHOW STATS FOR test_show_stats",
ImmutableMultiset.builder()
.addCopies(GET_TABLE, 9)
.addCopies(GET_TABLE, 5)
.build());
}

Expand All @@ -161,7 +161,7 @@ public void testShowStatsForTableWithFilter()

assertMetastoreInvocations("SHOW STATS FOR (SELECT * FROM test_show_stats_with_filter where age >= 2)",
ImmutableMultiset.builder()
.addCopies(GET_TABLE, 9)
.addCopies(GET_TABLE, 5)
.build());
}

Expand Down