Support write operations for S3 Tables in Iceberg#24994
Conversation
ebyhr
force-pushed
the
ebi/iceberg-s3tables-write
branch
from
2cdbe89 to
96b0d68
Compare
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java
Outdated
Show resolved
Hide resolved
lib/trino-filesystem/src/main/java/io/trino/filesystem/Locations.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/TrinoJdbcCatalog.java
Outdated
Show resolved
Hide resolved
ebyhr
force-pushed
the
ebi/iceberg-s3tables-write
branch
2 times, most recently
from
6f1f066 to
a0b4b48
Compare
ebyhr
force-pushed
the
ebi/iceberg-s3tables-write
branch
from
a0b4b48 to
679e686
Compare
|
Why no release notes entry? |
|
@mosabua We already have a release note entry for S3 Tables. There is no need to add another entry. |
Oh .. fair enough .. I should probably modify to remove the "reading" .. because now we also support writing ;-) |
| assertThat(Locations.isS3Tables("s3://75fed916-b871-4909-mx9t6iohbseks57q16e5y6nf1c8gguse2b--table-s3")).isTrue(); | ||
|
|
||
| assertThat(Locations.isS3Tables("s3://e97725d9-dbfb-4334-784sox7edps35ncq16arh546frqa1use2b--table-s3/")).isFalse(); | ||
| assertThat(Locations.isS3Tables("s3://75fed916-b871-4909-mx9t6iohbseks57q16e5y6nf1c8gguse2b--table-s3/")).isFalse(); |
There was a problem hiding this comment.
Why is this false?
Why Locations.isS3Tables("s3://xxxxxxx--table-s3/foo-bar") is also false?
it looks like isS3Tables works only for the top of the bucket, which is not intuitive
|
|
||
| if (!computedStatistics.isEmpty()) { | ||
| if (isS3Tables(icebergTable.location())) { | ||
| log.debug("S3 Tables does not support statistics: %s", table.name()); |
There was a problem hiding this comment.
That shouldn't be checked in finishInsert. Rather, getStatisticsCollectionMetadataForWrite should instruct the engine not to bother collecting stats, if they are not going to be persisted.
| void updateViewColumnComment(ConnectorSession session, SchemaTableName schemaViewName, String columnName, Optional<String> comment); | ||
|
|
||
| @Nullable | ||
| String defaultTableLocation(ConnectorSession session, SchemaTableName schemaTableName); |
There was a problem hiding this comment.
defaultTableLocation should be changed to return Optional
(ideally Optional<Location> rather than Optional<String>)
4 participants
Description
Locationclass requires a small change because it doesn't allow S3 Tables path, e.g.s3://{random}--table-s3'.Fixes #24916
cc: @jamesbornholt
Release notes
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.