-
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
Add Iceberg query runner with MinIO and file metastore #16861
Conversation
"http-server.http.port", "8080")) | ||
.setIcebergProperties(Map.of( | ||
"iceberg.catalog.type", "TESTING_FILE_METASTORE", | ||
"hive.metastore.catalog.dir", "s3://%s/".formatted(bucketName), |
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.
Is it intentional that hive.metastore.catalog.dir
is set to be in the very same bucket as the data for the iceberg tables?
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.
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.
That's actually required for managed tables:
trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java
Lines 351 to 355 in c7e8a6d
else if (table.getTableType().equals(MANAGED_TABLE.name())) { | |
if (!(new Path(table.getStorage().getLocation()).toString().contains(tableMetadataDirectory.toString()))) { | |
throw new TrinoException(HIVE_METASTORE_ERROR, "Table directory must be " + tableMetadataDirectory); | |
} | |
} |
Since Iceberg tables are external, we could in theory put them elsewhere, but by default, they live in the catalog location. The file hive metastore is designed to store metadata as dot files alongside the table data. For Iceberg, they end up in different directories due to using unique names for Iceberg.
I'm open to revisiting this if you think there's a need, but since this is for testing, I doubt it's worthwhile.
.build()) | ||
.build(); | ||
|
||
Thread.sleep(10); |
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 believe the sleep is redundant (at least ATQF won't sleep after creating a query runner, so it works without a sleep in tests).
will remove it in #16714, where i was adding similar runner
Release notes
(x) This is not user-visible or docs only and no release notes are required.