From 40a9a9ab2555ad0405b4c94748965762149d09a1 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Thu, 9 Jun 2022 16:21:19 +0200 Subject: [PATCH 1/2] Invoke main IcebergTableHandle constructor Don't invoke `IcebergTableHandle` constructor meant for deserialization explicitly. This fixes potential information loss in `IcebergMetadata.applyFilter` (it's believed it doesn't matter). To prevent future mistakes, the deserialization constructor is changed to a factory method. --- .../java/io/trino/plugin/iceberg/IcebergMetadata.java | 11 ++++++++--- .../io/trino/plugin/iceberg/IcebergTableHandle.java | 5 +++-- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java index 77048ae3d89d..b355aa18a833 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java @@ -325,7 +325,9 @@ public IcebergTableHandle getTableHandle( table.location(), table.properties(), NO_RETRIES, - ImmutableList.of()); + ImmutableList.of(), + false, + Optional.empty()); } @Override @@ -1716,7 +1718,8 @@ public Optional> applyFilter(C } return Optional.of(new ConstraintApplicationResult<>( - new IcebergTableHandle(table.getSchemaName(), + new IcebergTableHandle( + table.getSchemaName(), table.getTableName(), table.getTableType(), table.getSnapshotId(), @@ -1730,7 +1733,9 @@ public Optional> applyFilter(C table.getTableLocation(), table.getStorageProperties(), table.getRetryMode(), - table.getUpdatedColumns()), + table.getUpdatedColumns(), + table.isRecordScannedFiles(), + table.getMaxScannedFileSize()), remainingConstraint.transformKeys(ColumnHandle.class::cast), false)); } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableHandle.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableHandle.java index e2076e529a2d..64f6988d7ee3 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableHandle.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableHandle.java @@ -65,7 +65,8 @@ public class IcebergTableHandle private final Optional maxScannedFileSize; @JsonCreator - public IcebergTableHandle( + @Deprecated // For JSON deserialization only + public static IcebergTableHandle fromJson( @JsonProperty("schemaName") String schemaName, @JsonProperty("tableName") String tableName, @JsonProperty("tableType") TableType tableType, @@ -82,7 +83,7 @@ public IcebergTableHandle( @JsonProperty("retryMode") RetryMode retryMode, @JsonProperty("updatedColumns") List updatedColumns) { - this( + return new IcebergTableHandle( schemaName, tableName, tableType, From c3d1bb022815b0a9225645ba18f8a2038d0a6fdb Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Thu, 9 Jun 2022 16:19:29 +0200 Subject: [PATCH 2/2] Make IcebergTableHandle.enforcedPredicate coordinator-only The property is not used on workers (and shouldn't be, as it's the enforced one). Don't send it there. --- .../plugin/iceberg/IcebergTableHandle.java | 51 ++++++++++++++++--- 1 file changed, 45 insertions(+), 6 deletions(-) diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableHandle.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableHandle.java index 64f6988d7ee3..24ea57f450bc 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableHandle.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableHandle.java @@ -54,8 +54,8 @@ public class IcebergTableHandle // Filter used during split generation and table scan, but not required to be strictly enforced by Iceberg Connector private final TupleDomain unenforcedPredicate; - // Filter guaranteed to be enforced by Iceberg connector - private final TupleDomain enforcedPredicate; + // Filter guaranteed to be enforced by Iceberg connector. Coordinator-only + private final Optional> enforcedPredicate; private final Set projectedColumns; private final Optional nameMappingJson; @@ -75,7 +75,6 @@ public static IcebergTableHandle fromJson( @JsonProperty("partitionSpecJson") String partitionSpecJson, @JsonProperty("formatVersion") int formatVersion, @JsonProperty("unenforcedPredicate") TupleDomain unenforcedPredicate, - @JsonProperty("enforcedPredicate") TupleDomain enforcedPredicate, @JsonProperty("projectedColumns") Set projectedColumns, @JsonProperty("nameMappingJson") Optional nameMappingJson, @JsonProperty("tableLocation") String tableLocation, @@ -92,7 +91,7 @@ public static IcebergTableHandle fromJson( partitionSpecJson, formatVersion, unenforcedPredicate, - enforcedPredicate, + Optional.empty(), projectedColumns, nameMappingJson, tableLocation, @@ -121,6 +120,45 @@ public IcebergTableHandle( List updatedColumns, boolean recordScannedFiles, Optional maxScannedFileSize) + { + this( + schemaName, + tableName, + tableType, + snapshotId, + tableSchemaJson, + partitionSpecJson, + formatVersion, + unenforcedPredicate, + Optional.of(enforcedPredicate), + projectedColumns, + nameMappingJson, + tableLocation, + storageProperties, + retryMode, + updatedColumns, + recordScannedFiles, + maxScannedFileSize); + } + + private IcebergTableHandle( + String schemaName, + String tableName, + TableType tableType, + Optional snapshotId, + String tableSchemaJson, + String partitionSpecJson, + int formatVersion, + TupleDomain unenforcedPredicate, + Optional> enforcedPredicate, + Set projectedColumns, + Optional nameMappingJson, + String tableLocation, + Map storageProperties, + RetryMode retryMode, + List updatedColumns, + boolean recordScannedFiles, + Optional maxScannedFileSize) { this.schemaName = requireNonNull(schemaName, "schemaName is null"); this.tableName = requireNonNull(tableName, "tableName is null"); @@ -189,10 +227,11 @@ public TupleDomain getUnenforcedPredicate() return unenforcedPredicate; } - @JsonProperty + // do not serialize, not needed on workers + @JsonIgnore public TupleDomain getEnforcedPredicate() { - return enforcedPredicate; + return enforcedPredicate.orElseThrow(() -> new IllegalStateException("enforcedPredicate not set")); } @JsonProperty