Conversation
ebyhr
force-pushed
the
ebi/iceberg-s3tables
branch
from
0ccd911 to
0170e05
Compare
| { | ||
| private IcebergS3TablesQueryRunnerMain() {} | ||
|
|
||
| static void main() |
| public boolean isS3Tables() | ||
| { | ||
| String warehouse = restSessionCatalog.properties().get(CatalogProperties.WAREHOUSE_LOCATION); | ||
| return warehouse != null && warehouse.startsWith("s3tablescatalog/") && "sigv4".equals(restSessionCatalog.properties().get("rest.auth.type")); |
There was a problem hiding this comment.
Worth code comment where did s3tablescatalog come from?
|
|
||
| private boolean isS3Tables() | ||
| { | ||
| return catalog instanceof TrinoRestCatalog restCatalog && restCatalog.isS3Tables(); |
There was a problem hiding this comment.
Now that we have this new check, do we still need the Locations.isS3Tables check?
There was a problem hiding this comment.
The name "isS3Tables" is not clear to me, the method is using a catalog to check if it "isS3Tables", I don’t have a strong suggestion for an alternative name though
|
|
||
| if (isS3Tables()) { | ||
| // S3 Tables throw "Malformed request: Cannot parse missing field: statistics" error when we try to commit extended statistics | ||
| return TableStatisticsMetadata.empty(); |
There was a problem hiding this comment.
statistics is part of Iceberg spec for a while now, it's sad it's not supported by s3tables, because it makes queries inherently less performant on top of these tables. cc @pettyjamesm
Is there a public tracker link for this important and missing functionality?
(i mean AWS's issue, not an issue in Trino)
|
|
||
| @Nullable | ||
| String defaultTableLocation(ConnectorSession session, SchemaTableName schemaTableName); | ||
| Optional<String> defaultTableLocation(ConnectorSession session, SchemaTableName schemaTableName); |
|
|
||
| private boolean isS3Tables() | ||
| { | ||
| return catalog instanceof TrinoRestCatalog restCatalog && restCatalog.isS3Tables(); |
There was a problem hiding this comment.
The name "isS3Tables" is not clear to me, the method is using a catalog to check if it "isS3Tables", I don’t have a strong suggestion for an alternative name though
| File expectedSchemaDirectory = new File(tmpDirectory.toFile(), namespace + ".db"); | ||
| File expectedTableDirectory = new File(expectedSchemaDirectory, schemaTableName.getTableName()); | ||
| assertThat(catalogWithDefaultLocation.defaultTableLocation(SESSION, schemaTableName)).isEqualTo(expectedTableDirectory.toPath().toAbsolutePath().toString()); | ||
| assertThat(catalogWithDefaultLocation.defaultTableLocation(SESSION, schemaTableName)).contains(expectedTableDirectory.toPath().toAbsolutePath().toString()); |
There was a problem hiding this comment.
I think use "hasValue" is more readable for asserting Optional value
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
ebyhr
added
stale-ignore
3 participants
Description
Follow-up of #24994
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.