diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java index 42d385699d10..77779cd9bc2c 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java @@ -15,6 +15,7 @@ import io.airlift.configuration.Config; import io.airlift.configuration.ConfigDescription; +import io.airlift.configuration.DefunctConfig; import io.airlift.configuration.LegacyConfig; import io.airlift.units.DataSize; import io.airlift.units.Duration; @@ -35,6 +36,7 @@ import static java.util.concurrent.TimeUnit.DAYS; import static java.util.concurrent.TimeUnit.SECONDS; +@DefunctConfig("iceberg.allow-legacy-snapshot-syntax") public class IcebergConfig { public static final int FORMAT_VERSION_SUPPORT_MIN = 1; @@ -64,7 +66,6 @@ public class IcebergConfig // to avoid deleting those files if Trino is unable to check. private boolean deleteSchemaLocationsFallback; private double minimumAssignedSplitWeight = 0.05; - private boolean allowLegacySnapshotSyntax; private Optional materializedViewsStorageSchema = Optional.empty(); public CatalogType getCatalogType() @@ -308,20 +309,6 @@ public double getMinimumAssignedSplitWeight() return minimumAssignedSplitWeight; } - @Config("iceberg.allow-legacy-snapshot-syntax") - @Deprecated - public IcebergConfig setAllowLegacySnapshotSyntax(boolean allowLegacySnapshotSyntax) - { - this.allowLegacySnapshotSyntax = allowLegacySnapshotSyntax; - return this; - } - - @Deprecated - public boolean isAllowLegacySnapshotSyntax() - { - return allowLegacySnapshotSyntax; - } - @NotNull public Optional getMaterializedViewsStorageSchema() { 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 1ae1a18d5339..17cd4c22b6b1 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 @@ -145,7 +145,6 @@ import java.util.Optional; import java.util.OptionalLong; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; import java.util.function.Supplier; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -188,7 +187,6 @@ import static io.trino.plugin.iceberg.IcebergMetadataColumn.isMetadataColumnId; import static io.trino.plugin.iceberg.IcebergSessionProperties.getExpireSnapshotMinRetention; import static io.trino.plugin.iceberg.IcebergSessionProperties.getRemoveOrphanFilesMinRetention; -import static io.trino.plugin.iceberg.IcebergSessionProperties.isAllowLegacySnapshotSyntax; import static io.trino.plugin.iceberg.IcebergSessionProperties.isExtendedStatisticsEnabled; import static io.trino.plugin.iceberg.IcebergSessionProperties.isProjectionPushdownEnabled; import static io.trino.plugin.iceberg.IcebergSessionProperties.isStatisticsEnabled; @@ -220,7 +218,6 @@ import static io.trino.plugin.iceberg.procedure.IcebergTableProcedureId.EXPIRE_SNAPSHOTS; import static io.trino.plugin.iceberg.procedure.IcebergTableProcedureId.OPTIMIZE; import static io.trino.plugin.iceberg.procedure.IcebergTableProcedureId.REMOVE_ORPHAN_FILES; -import static io.trino.spi.StandardErrorCode.GENERIC_USER_ERROR; import static io.trino.spi.StandardErrorCode.INVALID_ANALYZE_PROPERTY; import static io.trino.spi.StandardErrorCode.INVALID_ARGUMENTS; import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED; @@ -265,8 +262,6 @@ public class IcebergMetadata private final TrinoCatalog catalog; private final TrinoFileSystemFactory fileSystemFactory; - private final Map snapshotIds = new ConcurrentHashMap<>(); - private Transaction transaction; public IcebergMetadata( @@ -332,16 +327,11 @@ public IcebergTableHandle getTableHandle( return null; } - if (name.getSnapshotId().isPresent() && endVersion.isPresent()) { - throw new TrinoException(GENERIC_USER_ERROR, "Cannot specify end version both in table name and FOR clause"); - } - Optional tableSnapshotId; Schema tableSchema; Optional partitionSpec; - if (endVersion.isPresent() || name.getSnapshotId().isPresent()) { - long snapshotId = endVersion.map(connectorTableVersion -> getSnapshotIdFromVersion(table, connectorTableVersion)) - .orElseGet(() -> resolveSnapshotId(table, name.getSnapshotId().get(), isAllowLegacySnapshotSyntax(session))); + if (endVersion.isPresent()) { + long snapshotId = getSnapshotIdFromVersion(table, endVersion.get()); tableSnapshotId = Optional.of(snapshotId); tableSchema = schemaFor(table, snapshotId); partitionSpec = Optional.empty(); @@ -436,21 +426,15 @@ private Optional getRawSystemTable(ConnectorSession session, Schema // Handled above. break; case HISTORY: - if (name.getSnapshotId().isPresent()) { - throw new TrinoException(NOT_SUPPORTED, "Snapshot ID not supported for history table: " + systemTableName); - } return Optional.of(new HistoryTable(systemTableName, table)); case SNAPSHOTS: - if (name.getSnapshotId().isPresent()) { - throw new TrinoException(NOT_SUPPORTED, "Snapshot ID not supported for snapshots table: " + systemTableName); - } return Optional.of(new SnapshotsTable(systemTableName, typeManager, table)); case PARTITIONS: - return Optional.of(new PartitionTable(systemTableName, typeManager, table, getSnapshotId(table, name.getSnapshotId(), isAllowLegacySnapshotSyntax(session)))); + return Optional.of(new PartitionTable(systemTableName, typeManager, table, getCurrentSnapshotId(table))); case MANIFESTS: - return Optional.of(new ManifestsTable(systemTableName, table, getSnapshotId(table, name.getSnapshotId(), isAllowLegacySnapshotSyntax(session)))); + return Optional.of(new ManifestsTable(systemTableName, table, getCurrentSnapshotId(table))); case FILES: - return Optional.of(new FilesTable(systemTableName, typeManager, table, getSnapshotId(table, name.getSnapshotId(), isAllowLegacySnapshotSyntax(session)))); + return Optional.of(new FilesTable(systemTableName, typeManager, table, getCurrentSnapshotId(table))); case PROPERTIES: return Optional.of(new PropertiesTable(systemTableName, table)); } @@ -2090,19 +2074,9 @@ public void setTableAuthorization(ConnectorSession session, SchemaTableName tabl catalog.setTablePrincipal(session, tableName, principal); } - private Optional getSnapshotId(Table table, Optional snapshotId, boolean allowLegacySnapshotSyntax) - { - // table.name() is an encoded version of SchemaTableName - return snapshotId - .map(id -> resolveSnapshotId(table, id, allowLegacySnapshotSyntax)) - .or(() -> Optional.ofNullable(table.currentSnapshot()).map(Snapshot::snapshotId)); - } - - private long resolveSnapshotId(Table table, long id, boolean allowLegacySnapshotSyntax) + private Optional getCurrentSnapshotId(Table table) { - return snapshotIds.computeIfAbsent( - table.name() + "@" + id, - ignored -> IcebergUtil.resolveSnapshotId(table, id, allowLegacySnapshotSyntax)); + return Optional.ofNullable(table.currentSnapshot()).map(Snapshot::snapshotId); } Table getIcebergTable(ConnectorSession session, SchemaTableName schemaTableName) diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSessionProperties.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSessionProperties.java index 2d4fb4f93563..49365ca78e4d 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSessionProperties.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSessionProperties.java @@ -78,7 +78,6 @@ public final class IcebergSessionProperties private static final String MINIMUM_ASSIGNED_SPLIT_WEIGHT = "minimum_assigned_split_weight"; public static final String EXPIRE_SNAPSHOTS_MIN_RETENTION = "expire_snapshots_min_retention"; public static final String REMOVE_ORPHAN_FILES_MIN_RETENTION = "remove_orphan_files_min_retention"; - private static final String ALLOW_LEGACY_SNAPSHOT_SYNTAX = "allow_legacy_snapshot_syntax"; private final List> sessionProperties; @@ -253,11 +252,6 @@ public IcebergSessionProperties( "Minimal retention period for remove_orphan_files procedure", icebergConfig.getRemoveOrphanFilesMinRetention(), false)) - .add(booleanProperty( - ALLOW_LEGACY_SNAPSHOT_SYNTAX, - "Allow snapshot access based on timestamp and snapshotid", - icebergConfig.isAllowLegacySnapshotSyntax(), - false)) .build(); } @@ -423,9 +417,4 @@ public static double getMinimumAssignedSplitWeight(ConnectorSession session) { return session.getProperty(MINIMUM_ASSIGNED_SPLIT_WEIGHT, Double.class); } - - public static boolean isAllowLegacySnapshotSyntax(ConnectorSession session) - { - return session.getProperty(ALLOW_LEGACY_SNAPSHOT_SYNTAX, Boolean.class); - } } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableName.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableName.java index 118a340ca9a9..f3c3a5ab62c8 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableName.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableName.java @@ -16,12 +16,10 @@ import io.trino.spi.TrinoException; import java.util.Locale; -import java.util.Optional; import java.util.regex.Matcher; import java.util.regex.Pattern; import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED; -import static java.lang.Long.parseLong; import static java.lang.String.format; import static java.util.Objects.requireNonNull; @@ -29,18 +27,15 @@ public class IcebergTableName { private static final Pattern TABLE_PATTERN = Pattern.compile("" + "(?[^$@]+)" + - "(?:@(?[0-9]+))?" + - "(?:\\$(?[^@]+)(?:@(?[0-9]+))?)?"); + "(?:\\$(?[^@]+))?"); private final String tableName; private final TableType tableType; - private final Optional snapshotId; - public IcebergTableName(String tableName, TableType tableType, Optional snapshotId) + public IcebergTableName(String tableName, TableType tableType) { this.tableName = requireNonNull(tableName, "tableName is null"); this.tableType = requireNonNull(tableType, "tableType is null"); - this.snapshotId = requireNonNull(snapshotId, "snapshotId is null"); } public String getTableName() @@ -53,11 +48,6 @@ public TableType getTableType() return tableType; } - public Optional getSnapshotId() - { - return snapshotId; - } - public String getTableNameWithType() { return tableName + "$" + tableType.name().toLowerCase(Locale.ROOT); @@ -66,7 +56,7 @@ public String getTableNameWithType() @Override public String toString() { - return getTableNameWithType() + "@" + snapshotId; + return getTableNameWithType(); } public static IcebergTableName from(String name) @@ -78,8 +68,6 @@ public static IcebergTableName from(String name) String table = match.group("table"); String typeString = match.group("type"); - String ver1 = match.group("ver1"); - String ver2 = match.group("ver2"); TableType type = TableType.DATA; if (typeString != null) { @@ -91,22 +79,6 @@ public static IcebergTableName from(String name) } } - Optional version = Optional.empty(); - if (type == TableType.DATA || type == TableType.PARTITIONS || type == TableType.MANIFESTS || type == TableType.FILES) { - if (ver1 != null && ver2 != null) { - throw new TrinoException(NOT_SUPPORTED, "Invalid Iceberg table name (cannot specify two @ versions): " + name); - } - if (ver1 != null) { - version = Optional.of(parseLong(ver1)); - } - else if (ver2 != null) { - version = Optional.of(parseLong(ver2)); - } - } - else if (ver1 != null || ver2 != null) { - throw new TrinoException(NOT_SUPPORTED, format("Invalid Iceberg table name (cannot use @ version with table type '%s'): %s", type, name)); - } - - return new IcebergTableName(table, type, version); + return new IcebergTableName(table, type); } } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergUtil.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergUtil.java index f72c282f0e99..512c5ef2331c 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergUtil.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergUtil.java @@ -83,12 +83,10 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static com.google.common.collect.Lists.reverse; import static io.airlift.slice.Slices.utf8Slice; import static io.trino.plugin.hive.HiveMetadata.TABLE_COMMENT; import static io.trino.plugin.iceberg.ColumnIdentity.createColumnIdentity; import static io.trino.plugin.iceberg.IcebergErrorCode.ICEBERG_INVALID_PARTITION_VALUE; -import static io.trino.plugin.iceberg.IcebergErrorCode.ICEBERG_INVALID_SNAPSHOT_ID; import static io.trino.plugin.iceberg.IcebergMetadata.ORC_BLOOM_FILTER_COLUMNS_KEY; import static io.trino.plugin.iceberg.IcebergMetadata.ORC_BLOOM_FILTER_FPP_KEY; import static io.trino.plugin.iceberg.IcebergTableProperties.FILE_FORMAT_PROPERTY; @@ -217,31 +215,6 @@ public static Map getIcebergTableProperties(Table icebergTable) return properties.buildOrThrow(); } - @Deprecated - public static long resolveSnapshotId(Table table, long snapshotId, boolean allowLegacySnapshotSyntax) - { - if (!allowLegacySnapshotSyntax) { - throw new TrinoException( - NOT_SUPPORTED, - format( - "Failed to access snapshot %s for table %s. This syntax for accessing Iceberg tables is not " - + "supported. Use the AS OF syntax OR set the catalog session property " - + "allow_legacy_snapshot_syntax=true for temporarily restoring previous behavior.", - snapshotId, - table.name())); - } - - if (table.snapshot(snapshotId) != null) { - return snapshotId; - } - - return reverse(table.history()).stream() - .filter(entry -> entry.timestampMillis() <= snapshotId) - .map(HistoryEntry::snapshotId) - .findFirst() - .orElseThrow(() -> new TrinoException(ICEBERG_INVALID_SNAPSHOT_ID, format("Invalid snapshot [%s] for table: %s", snapshotId, table))); - } - public static List getColumns(Schema schema, TypeManager typeManager) { return schema.columns().stream() diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java index cd687c220560..b8206ac03e15 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java @@ -71,7 +71,6 @@ import java.util.Optional; import java.util.OptionalInt; import java.util.Set; -import java.util.concurrent.TimeUnit; import java.util.function.Consumer; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -84,7 +83,6 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.collect.Iterables.concat; -import static com.google.common.collect.Iterables.getLast; import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.common.collect.MoreCollectors.onlyElement; import static com.google.common.util.concurrent.Uninterruptibles.sleepUninterruptibly; @@ -4561,16 +4559,13 @@ public void testOptimizeCleansUpDeleteFiles() @Test public void testOptimizeSnapshot() { - Session session = Session.builder(getSession()) - .setCatalogSessionProperty(getSession().getCatalog().orElseThrow(), "allow_legacy_snapshot_syntax", "true") - .build(); String tableName = "test_optimize_snapshot_" + randomTableSuffix(); assertUpdate("CREATE TABLE " + tableName + " (a) AS VALUES 11", 1); long snapshotId = getCurrentSnapshotId(tableName); assertUpdate("INSERT INTO " + tableName + " VALUES 22", 1); - assertThatThrownBy(() -> query(session, "ALTER TABLE \"%s@%d\" EXECUTE OPTIMIZE".formatted(tableName, snapshotId))) - .hasMessage("Cannot execute table procedure OPTIMIZE on old snapshot " + snapshotId); + assertThatThrownBy(() -> query("ALTER TABLE \"%s@%d\" EXECUTE OPTIMIZE".formatted(tableName, snapshotId))) + .hasMessage(format("Invalid Iceberg table name: %s@%d", tableName, snapshotId)); assertThat(query("SELECT * FROM " + tableName)) .matches("VALUES 11, 22"); @@ -4918,16 +4913,13 @@ public void testExpireSnapshotsPartitionedTable() @Test public void testExpireSnapshotsOnSnapshot() { - Session session = Session.builder(getSession()) - .setCatalogSessionProperty(getSession().getCatalog().orElseThrow(), "allow_legacy_snapshot_syntax", "true") - .build(); String tableName = "test_expire_snapshots_on_snapshot_" + randomTableSuffix(); assertUpdate("CREATE TABLE " + tableName + " (a) AS VALUES 11", 1); long snapshotId = getCurrentSnapshotId(tableName); assertUpdate("INSERT INTO " + tableName + " VALUES 22", 1); - assertThatThrownBy(() -> query(session, "ALTER TABLE \"%s@%d\" EXECUTE EXPIRE_SNAPSHOTS".formatted(tableName, snapshotId))) - .hasMessage("Cannot execute table procedure EXPIRE_SNAPSHOTS on old snapshot " + snapshotId); + assertThatThrownBy(() -> query("ALTER TABLE \"%s@%d\" EXECUTE EXPIRE_SNAPSHOTS".formatted(tableName, snapshotId))) + .hasMessage(format("Invalid Iceberg table name: %s@%d", tableName, snapshotId)); assertThat(query("SELECT * FROM " + tableName)) .matches("VALUES 11, 22"); @@ -5098,16 +5090,13 @@ public void testRemoveOrphanFilesParameterValidation() @Test public void testRemoveOrphanFilesOnSnapshot() { - Session session = Session.builder(getSession()) - .setCatalogSessionProperty(getSession().getCatalog().orElseThrow(), "allow_legacy_snapshot_syntax", "true") - .build(); String tableName = "test_remove_orphan_files_on_snapshot_" + randomTableSuffix(); assertUpdate("CREATE TABLE " + tableName + " (a) AS VALUES 11", 1); long snapshotId = getCurrentSnapshotId(tableName); assertUpdate("INSERT INTO " + tableName + " VALUES 22", 1); - assertThatThrownBy(() -> query(session, "ALTER TABLE \"%s@%d\" EXECUTE REMOVE_ORPHAN_FILES".formatted(tableName, snapshotId))) - .hasMessage("Cannot execute table procedure REMOVE_ORPHAN_FILES on old snapshot " + snapshotId); + assertThatThrownBy(() -> query("ALTER TABLE \"%s@%d\" EXECUTE REMOVE_ORPHAN_FILES".formatted(tableName, snapshotId))) + .hasMessage(format("Invalid Iceberg table name: %s@%d", tableName, snapshotId)); assertThat(query("SELECT * FROM " + tableName)) .matches("VALUES 11, 22"); @@ -5243,29 +5232,27 @@ public void testEmptyDelete() @Test public void testModifyingOldSnapshotIsNotPossible() { - Session sessionWithLegacySyntaxSupport = Session.builder(getSession()) - .setCatalogSessionProperty("iceberg", "allow_legacy_snapshot_syntax", "true") - .build(); String tableName = "test_modifying_old_snapshot_" + randomTableSuffix(); assertUpdate(format("CREATE TABLE %s (col int)", tableName)); assertUpdate(format("INSERT INTO %s VALUES 1,2,3", tableName), 3); long oldSnapshotId = getCurrentSnapshotId(tableName); assertUpdate(format("INSERT INTO %s VALUES 4,5,6", tableName), 3); - assertQuery(sessionWithLegacySyntaxSupport, format("SELECT * FROM \"%s@%d\"", tableName, oldSnapshotId), "VALUES 1,2,3"); - assertThatThrownBy(() -> query(sessionWithLegacySyntaxSupport, format("INSERT INTO \"%s@%d\" VALUES 7,8,9", tableName, oldSnapshotId))) - .hasMessage("Modifying old snapshot is not supported in Iceberg"); - assertThatThrownBy(() -> query(sessionWithLegacySyntaxSupport, format("DELETE FROM \"%s@%d\" WHERE col = 5", tableName, oldSnapshotId))) - .hasMessage("Modifying old snapshot is not supported in Iceberg"); - assertThatThrownBy(() -> query(sessionWithLegacySyntaxSupport, format("UPDATE \"%s@%d\" SET col = 50 WHERE col = 5", tableName, oldSnapshotId))) - .hasMessage("Modifying old snapshot is not supported in Iceberg"); - // TODO Change to assertThatThrownBy because the syntax `table@versionid` should not be supported for DML operations - assertUpdate(sessionWithLegacySyntaxSupport, format("INSERT INTO \"%s@%d\" VALUES 7,8,9", tableName, getCurrentSnapshotId(tableName)), 3); - assertUpdate(sessionWithLegacySyntaxSupport, format("DELETE FROM \"%s@%d\" WHERE col = 9", tableName, getCurrentSnapshotId(tableName)), 1); - assertThatThrownBy(() -> assertUpdate(sessionWithLegacySyntaxSupport, format("UPDATE \"%s@%d\" set col = 50 WHERE col = 5", tableName, getCurrentSnapshotId(tableName)))) - .hasMessage("Partition spec missing in the table handle"); - // TODO Change to assertThatThrownBy because the syntax `table@versionid` should not be supported for DML operations - assertQuerySucceeds(sessionWithLegacySyntaxSupport, format("ALTER TABLE \"%s@%d\" EXECUTE OPTIMIZE", tableName, getCurrentSnapshotId(tableName))); - assertQuery(format("SELECT * FROM %s", tableName), "VALUES 1,2,3,4,5,6,7,8"); + assertQuery(format("SELECT * FROM %s FOR VERSION AS OF %d", tableName, oldSnapshotId), "VALUES 1,2,3"); + assertThatThrownBy(() -> query(format("INSERT INTO \"%s@%d\" VALUES 7,8,9", tableName, oldSnapshotId))) + .hasMessage(format("Invalid Iceberg table name: %s@%d", tableName, oldSnapshotId)); + assertThatThrownBy(() -> query(format("DELETE FROM \"%s@%d\" WHERE col = 5", tableName, oldSnapshotId))) + .hasMessage(format("Invalid Iceberg table name: %s@%d", tableName, oldSnapshotId)); + assertThatThrownBy(() -> query(format("UPDATE \"%s@%d\" SET col = 50 WHERE col = 5", tableName, oldSnapshotId))) + .hasMessage(format("Invalid Iceberg table name: %s@%d", tableName, oldSnapshotId)); + assertThatThrownBy(() -> query(format("INSERT INTO \"%s@%d\" VALUES 7,8,9", tableName, getCurrentSnapshotId(tableName)))) + .hasMessage(format("Invalid Iceberg table name: %s@%d", tableName, getCurrentSnapshotId(tableName))); + assertThatThrownBy(() -> query(format("DELETE FROM \"%s@%d\" WHERE col = 9", tableName, getCurrentSnapshotId(tableName)))) + .hasMessage(format("Invalid Iceberg table name: %s@%d", tableName, getCurrentSnapshotId(tableName))); + assertThatThrownBy(() -> assertUpdate(format("UPDATE \"%s@%d\" set col = 50 WHERE col = 5", tableName, getCurrentSnapshotId(tableName)))) + .hasMessage(format("Invalid Iceberg table name: %s@%d", tableName, getCurrentSnapshotId(tableName))); + assertThatThrownBy(() -> query(format("ALTER TABLE \"%s@%d\" EXECUTE OPTIMIZE", tableName, oldSnapshotId))) + .hasMessage(format("Invalid Iceberg table name: %s@%d", tableName, oldSnapshotId)); + assertQuery(format("SELECT * FROM %s", tableName), "VALUES 1,2,3,4,5,6"); assertUpdate("DROP TABLE " + tableName); } @@ -5309,43 +5296,8 @@ public void testReadingFromSpecificSnapshot() assertUpdate(format("INSERT INTO %s VALUES(1, 1)", tableName), 1); List ids = getSnapshotsIdsByCreationOrder(tableName); - assertQuery(sessionWithLegacySyntaxSupport(), format("SELECT count(*) FROM \"%s@%d\"", tableName, ids.get(0)), "VALUES(0)"); - assertQuery(sessionWithLegacySyntaxSupport(), format("SELECT * FROM \"%s@%d\"", tableName, ids.get(1)), "VALUES(1,1)"); - assertUpdate(format("DROP TABLE %s", tableName)); - } - - @Test - public void testLegacySnapshotSyntaxSupport() - { - String tableName = "test_legacy_snapshot_access" + randomTableSuffix(); - assertUpdate(format("CREATE TABLE %s (a BIGINT, b BIGINT)", tableName)); - assertUpdate(format("INSERT INTO %s VALUES(1, 1)", tableName), 1); - List ids = getSnapshotsIdsByCreationOrder(tableName); - // come up with a timestamp value in future that is not an already existing id - long futureTimeStamp = System.currentTimeMillis() + TimeUnit.MINUTES.toMillis(5); - while (ids.contains(futureTimeStamp)) { - futureTimeStamp += TimeUnit.MINUTES.toMillis(5); - } - - String selectAllFromFutureTimeStamp = format("SELECT * FROM \"%s@%d\"", tableName, futureTimeStamp); - String selectAllFromLatestId = format("SELECT * FROM \"%s@%d\"", tableName, getLast(ids)); - String selectFromPartitionsTable = format("SELECT record_count FROM \"%s$partitions@%d\"", tableName, getLast(ids)); - - assertQuery(sessionWithLegacySyntaxSupport(), selectAllFromFutureTimeStamp, "VALUES(1, 1)"); - assertQuery(sessionWithLegacySyntaxSupport(), selectAllFromLatestId, "VALUES(1, 1)"); - assertQuery(sessionWithLegacySyntaxSupport(), selectFromPartitionsTable, "VALUES(1)"); - - // DISABLED - String errorMessage = "Failed to access snapshot .* for table .*. This syntax for accessing Iceberg tables is not " - + "supported. Use the AS OF syntax OR set the catalog session property " - + "allow_legacy_snapshot_syntax=true for temporarily restoring previous behavior."; - assertThatThrownBy(() -> query(getSession(), selectAllFromFutureTimeStamp)) - .hasMessageMatching(errorMessage); - assertThatThrownBy(() -> query(getSession(), selectAllFromLatestId)) - .hasMessageMatching(errorMessage); - assertThatThrownBy(() -> query(getSession(), selectFromPartitionsTable)) - .hasMessageMatching(errorMessage); - + assertQuery(format("SELECT count(*) FROM %s FOR VERSION AS OF %d", tableName, ids.get(0)), "VALUES(0)"); + assertQuery(format("SELECT * FROM %s FOR VERSION AS OF %d", tableName, ids.get(1)), "VALUES(1,1)"); assertUpdate(format("DROP TABLE %s", tableName)); } @@ -5361,8 +5313,7 @@ public void testSelectWithMoreThanOneSnapshotOfTheSameTable() assertQuery(format("SELECT * FROM %s", tableName), "SELECT * FROM (VALUES(1,1), (2,2), (3,3))"); assertQuery( - sessionWithLegacySyntaxSupport(), - format("SELECT * FROM %1$s EXCEPT (SELECT * FROM \"%1$s@%2$d\" EXCEPT SELECT * FROM \"%1$s@%3$d\")", tableName, ids.get(2), ids.get(1)), + format("SELECT * FROM %1$s EXCEPT (SELECT * FROM %1$s FOR VERSION AS OF %2$d EXCEPT SELECT * FROM %1$s FOR VERSION AS OF %3$d)", tableName, ids.get(2), ids.get(1)), "SELECT * FROM (VALUES(1,1), (3,3))"); assertUpdate(format("DROP TABLE %s", tableName)); } @@ -5905,11 +5856,4 @@ private List getSnapshotsIdsByCreationOrder(String tableName) .map(row -> (Long) row.getField(idField)) .collect(toList()); } - - private Session sessionWithLegacySyntaxSupport() - { - return Session.builder(getSession()) - .setCatalogSessionProperty("iceberg", "allow_legacy_snapshot_syntax", "true") - .build(); - } } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergAnalyze.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergAnalyze.java index aee239d6cb33..9102042c8ff1 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergAnalyze.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergAnalyze.java @@ -385,16 +385,13 @@ public void testAnalyzeSomeColumns() @Test public void testAnalyzeSnapshot() { - Session session = Session.builder(getSession()) - .setCatalogSessionProperty(getSession().getCatalog().orElseThrow(), "allow_legacy_snapshot_syntax", "true") - .build(); String tableName = "test_analyze_snapshot_" + randomTableSuffix(); assertUpdate("CREATE TABLE " + tableName + " (a) AS VALUES 11", 1); long snapshotId = getCurrentSnapshotId(tableName); assertUpdate("INSERT INTO " + tableName + " VALUES 22", 1); - assertThatThrownBy(() -> query(session, "ANALYZE \"%s@%d\"".formatted(tableName, snapshotId))) - .hasMessage("Cannot analyze old snapshot " + snapshotId); + assertThatThrownBy(() -> query("ANALYZE \"%s@%d\"".formatted(tableName, snapshotId))) + .hasMessage(format("Invalid Iceberg table name: %s@%d", tableName, snapshotId)); assertThat(query("SELECT * FROM " + tableName)) .matches("VALUES 11, 22"); @@ -490,16 +487,13 @@ public void testDropStatsAccessControl() @Test public void testDropStatsSnapshot() { - Session session = Session.builder(getSession()) - .setCatalogSessionProperty(getSession().getCatalog().orElseThrow(), "allow_legacy_snapshot_syntax", "true") - .build(); String tableName = "test_drop_stats_snapshot_" + randomTableSuffix(); assertUpdate("CREATE TABLE " + tableName + " (a) AS VALUES 11", 1); long snapshotId = getCurrentSnapshotId(tableName); assertUpdate("INSERT INTO " + tableName + " VALUES 22", 1); - assertThatThrownBy(() -> query(session, "ALTER TABLE \"%s@%d\" EXECUTE DROP_EXTENDED_STATS".formatted(tableName, snapshotId))) - .hasMessage("Cannot execute table procedure DROP_EXTENDED_STATS on old snapshot " + snapshotId); + assertThatThrownBy(() -> query("ALTER TABLE \"%s@%d\" EXECUTE DROP_EXTENDED_STATS".formatted(tableName, snapshotId))) + .hasMessage(format("Invalid Iceberg table name: %s@%d", tableName, snapshotId)); assertThat(query("SELECT * FROM " + tableName)) .matches("VALUES 11, 22"); diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConfig.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConfig.java index d83a158e99bb..d1f9f122d17f 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConfig.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConfig.java @@ -58,7 +58,6 @@ public void testDefaults() .setDeleteSchemaLocationsFallback(false) .setTargetMaxFileSize(DataSize.of(1, GIGABYTE)) .setMinimumAssignedSplitWeight(0.05) - .setAllowLegacySnapshotSyntax(false) .setMaterializedViewsStorageSchema(null)); } @@ -83,7 +82,6 @@ public void testExplicitPropertyMappings() .put("iceberg.delete-schema-locations-fallback", "true") .put("iceberg.target-max-file-size", "1MB") .put("iceberg.minimum-assigned-split-weight", "0.01") - .put("iceberg.allow-legacy-snapshot-syntax", "true") .put("iceberg.materialized-views.storage-schema", "mv_storage_schema") .buildOrThrow(); @@ -105,7 +103,6 @@ public void testExplicitPropertyMappings() .setDeleteSchemaLocationsFallback(true) .setTargetMaxFileSize(DataSize.of(1, MEGABYTE)) .setMinimumAssignedSplitWeight(0.01) - .setAllowLegacySnapshotSyntax(true) .setMaterializedViewsStorageSchema("mv_storage_schema"); assertFullMapping(properties, expected); diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergReadVersionedTable.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergReadVersionedTable.java index 806d6b9c5508..b06c4d03c4a3 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergReadVersionedTable.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergReadVersionedTable.java @@ -87,10 +87,10 @@ public void testSelectTableWithEndLongTimestampWithTimezone() public void testEndVersionInTableNameAndForClauseShouldFail() { assertQueryFails("SELECT * FROM \"test_iceberg_read_versioned_table@" + v1SnapshotId + "\" FOR VERSION AS OF " + v1SnapshotId, - "Cannot specify end version both in table name and FOR clause"); + "Invalid Iceberg table name: test_iceberg_read_versioned_table@%d".formatted(v1SnapshotId)); assertQueryFails("SELECT * FROM \"test_iceberg_read_versioned_table@" + v1SnapshotId + "\" FOR TIMESTAMP AS OF " + timestampLiteral(v1EpochMillis, 9), - "Cannot specify end version both in table name and FOR clause"); + "Invalid Iceberg table name: test_iceberg_read_versioned_table@%d".formatted(v1SnapshotId)); } @Test diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergTableName.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergTableName.java index 0d814b153407..9fc619b42307 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergTableName.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergTableName.java @@ -15,8 +15,6 @@ import org.testng.annotations.Test; -import java.util.Optional; - import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED; import static io.trino.testing.assertions.TrinoExceptionAssert.assertTrinoExceptionThrownBy; import static org.testng.Assert.assertEquals; @@ -26,22 +24,20 @@ public class TestIcebergTableName @Test public void testFrom() { - assertFrom("abc", "abc", TableType.DATA, Optional.empty()); - assertFrom("abc@123", "abc", TableType.DATA, Optional.of(123L)); - assertFrom("abc$data", "abc", TableType.DATA, Optional.empty()); - assertFrom("xyz@456", "xyz", TableType.DATA, Optional.of(456L)); - assertFrom("xyz$data@456", "xyz", TableType.DATA, Optional.of(456L)); - assertFrom("abc$partitions@456", "abc", TableType.PARTITIONS, Optional.of(456L)); - assertFrom("abc$manifests@456", "abc", TableType.MANIFESTS, Optional.of(456L)); - assertFrom("abc$manifests@456", "abc", TableType.MANIFESTS, Optional.of(456L)); - assertFrom("abc$history", "abc", TableType.HISTORY, Optional.empty()); - assertFrom("abc$snapshots", "abc", TableType.SNAPSHOTS, Optional.empty()); + assertFrom("abc", "abc", TableType.DATA); + assertFrom("abc$data", "abc", TableType.DATA); + assertFrom("abc$history", "abc", TableType.HISTORY); + assertFrom("abc$snapshots", "abc", TableType.SNAPSHOTS); + assertInvalid("abc@123", "Invalid Iceberg table name: abc@123"); assertInvalid("abc@xyz", "Invalid Iceberg table name: abc@xyz"); assertInvalid("abc$what", "Invalid Iceberg table name (unknown type 'what'): abc$what"); - assertInvalid("abc@123$data@456", "Invalid Iceberg table name (cannot specify two @ versions): abc@123$data@456"); - assertInvalid("abc@123$snapshots", "Invalid Iceberg table name (cannot use @ version with table type 'SNAPSHOTS'): abc@123$snapshots"); - assertInvalid("abc$snapshots@456", "Invalid Iceberg table name (cannot use @ version with table type 'SNAPSHOTS'): abc$snapshots@456"); + assertInvalid("abc@123$data@456", "Invalid Iceberg table name: abc@123$data@456"); + assertInvalid("abc@123$snapshots", "Invalid Iceberg table name: abc@123$snapshots"); + assertInvalid("abc$snapshots@456", "Invalid Iceberg table name: abc$snapshots@456"); + assertInvalid("xyz$data@456", "Invalid Iceberg table name: xyz$data@456"); + assertInvalid("abc$partitions@456", "Invalid Iceberg table name: abc$partitions@456"); + assertInvalid("abc$manifests@456", "Invalid Iceberg table name: abc$manifests@456"); } private static void assertInvalid(String inputName, String message) @@ -51,11 +47,10 @@ private static void assertInvalid(String inputName, String message) .hasMessage(message); } - private static void assertFrom(String inputName, String tableName, TableType tableType, Optional snapshotId) + private static void assertFrom(String inputName, String tableName, TableType tableType) { IcebergTableName name = IcebergTableName.from(inputName); assertEquals(name.getTableName(), tableName); assertEquals(name.getTableType(), tableType); - assertEquals(name.getSnapshotId(), snapshotId); } }