From 83ca21d75591f486e7d5c4cc3f04cef3216b81a8 Mon Sep 17 00:00:00 2001 From: Mayank Vadariya <48036907+mayankvadariya@users.noreply.github.com> Date: Mon, 28 Oct 2024 23:24:03 -0400 Subject: [PATCH] fixup! Add support for case-insensitive object names in Iceberg REST catalog - use single cache --- .../rest/TrinoIcebergRestCatalogFactory.java | 34 ++++++------ .../catalog/rest/TrinoRestCatalog.java | 54 +++++++++---------- .../catalog/rest/TestTrinoRestCatalog.java | 7 +-- 3 files changed, 45 insertions(+), 50 deletions(-) diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java index c81e9e0123b54..a71c5dad7e2b4 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java @@ -61,9 +61,8 @@ public class TrinoIcebergRestCatalogFactory private final boolean uniqueTableLocation; private final TypeManager typeManager; private final boolean caseInsensitiveNameMatching; - private final Cache remoteNamespaceMappingCache; - private final Cache remoteTableMappingCache; - private final Cache remoteViewMappingCache; + private final Optional> remoteNamespaceMappingCache; + private final Optional> remoteTableMappingCache; @GuardedBy("this") private RESTSessionCatalog icebergCatalog; @@ -93,18 +92,20 @@ public TrinoIcebergRestCatalogFactory( this.uniqueTableLocation = icebergConfig.isUniqueTableLocation(); this.typeManager = requireNonNull(typeManager, "typeManager is null"); this.caseInsensitiveNameMatching = restConfig.isCaseInsensitiveNameMatching(); - this.remoteNamespaceMappingCache = EvictableCacheBuilder.newBuilder() - .expireAfterWrite(restConfig.getCaseInsensitiveNameMatchingCacheTtl().toMillis(), MILLISECONDS) - .shareNothingWhenDisabled() - .build(); - this.remoteTableMappingCache = EvictableCacheBuilder.newBuilder() - .expireAfterWrite(restConfig.getCaseInsensitiveNameMatchingCacheTtl().toMillis(), MILLISECONDS) - .shareNothingWhenDisabled() - .build(); - this.remoteViewMappingCache = EvictableCacheBuilder.newBuilder() - .expireAfterWrite(restConfig.getCaseInsensitiveNameMatchingCacheTtl().toMillis(), MILLISECONDS) - .shareNothingWhenDisabled() - .build(); + this.remoteNamespaceMappingCache = + caseInsensitiveNameMatching + ? Optional.of(EvictableCacheBuilder.newBuilder() + .expireAfterWrite(restConfig.getCaseInsensitiveNameMatchingCacheTtl().toMillis(), MILLISECONDS) + .shareNothingWhenDisabled() + .build()) + : Optional.empty(); + this.remoteTableMappingCache = + caseInsensitiveNameMatching + ? Optional.of(EvictableCacheBuilder.newBuilder() + .expireAfterWrite(restConfig.getCaseInsensitiveNameMatchingCacheTtl().toMillis(), MILLISECONDS) + .shareNothingWhenDisabled() + .build()) + : Optional.empty(); } @Override @@ -152,7 +153,6 @@ public synchronized TrinoCatalog create(ConnectorIdentity identity) uniqueTableLocation, caseInsensitiveNameMatching, remoteNamespaceMappingCache, - remoteTableMappingCache, - remoteViewMappingCache); + remoteTableMappingCache); } } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java index 0b4e98dd168b2..28ec25db5a563 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java @@ -118,9 +118,8 @@ public class TrinoRestCatalog private final String trinoVersion; private final boolean useUniqueTableLocation; private final boolean caseInsensitiveNameMatching; - private final Cache remoteNamespaceMappingCache; - private final Cache remoteTableMappingCache; - private final Cache remoteViewMappingCache; + private final Optional> remoteNamespaceMappingCache; + private final Optional> remoteTableMappingCache; private final Cache tableCache = EvictableCacheBuilder.newBuilder() .maximumSize(PER_QUERY_CACHE_SIZE) @@ -136,9 +135,8 @@ public TrinoRestCatalog( TypeManager typeManager, boolean useUniqueTableLocation, boolean caseInsensitiveNameMatching, - Cache remoteNamespaceMappingCache, - Cache remoteTableMappingCache, - Cache remoteViewMappingCache) + Optional> remoteNamespaceMappingCache, + Optional> remoteTableMappingCache) { this.restSessionCatalog = requireNonNull(restSessionCatalog, "restSessionCatalog is null"); this.catalogName = requireNonNull(catalogName, "catalogName is null"); @@ -151,7 +149,8 @@ public TrinoRestCatalog( this.caseInsensitiveNameMatching = caseInsensitiveNameMatching; this.remoteNamespaceMappingCache = requireNonNull(remoteNamespaceMappingCache, "remoteNamespaceMappingCache is null"); this.remoteTableMappingCache = requireNonNull(remoteTableMappingCache, "remoteTableMappingCache is null"); - this.remoteViewMappingCache = requireNonNull(remoteViewMappingCache, "remoteViewMappingCache is null"); + checkArgument(caseInsensitiveNameMatching == remoteNamespaceMappingCache.isPresent()); + checkArgument(caseInsensitiveNameMatching == remoteTableMappingCache.isPresent()); } @Override @@ -194,7 +193,7 @@ public void dropNamespace(ConnectorSession session, String namespace) throw new TrinoException(ICEBERG_CATALOG_ERROR, format("Failed to drop namespace: %s", namespace), e); } if (caseInsensitiveNameMatching) { - remoteNamespaceMappingCache.invalidate(toNamespace(namespace)); + remoteNamespaceMappingCache.get().invalidate(toNamespace(namespace)); } } @@ -364,7 +363,7 @@ public void unregisterTable(ConnectorSession session, SchemaTableName tableName) } invalidateTableCache(tableName); if (caseInsensitiveNameMatching) { - remoteTableMappingCache.invalidate(toIdentifier(tableName)); + remoteTableMappingCache.get().invalidate(toIdentifier(tableName)); } } @@ -376,7 +375,7 @@ public void dropTable(ConnectorSession session, SchemaTableName schemaTableName) } invalidateTableCache(schemaTableName); if (caseInsensitiveNameMatching) { - remoteTableMappingCache.invalidate(toIdentifier(schemaTableName)); + remoteTableMappingCache.get().invalidate(toIdentifier(schemaTableName)); } } @@ -399,7 +398,7 @@ public void renameTable(ConnectorSession session, SchemaTableName from, SchemaTa } invalidateTableCache(from); if (caseInsensitiveNameMatching) { - remoteTableMappingCache.invalidate(toIdentifier(from)); + remoteTableMappingCache.get().invalidate(toIdentifier(from)); } } @@ -427,16 +426,17 @@ public Table loadTable(ConnectorSession session, SchemaTableName schemaTableName private TableIdentifier toRemoteObject(ConnectorSession session, SchemaTableName schemaTableName) { - TableIdentifier liveTable = toLiveRemoteTable(session, schemaTableName); - TableIdentifier liveView = toLiveRemoteView(session, schemaTableName); - if (!liveTable.name().equals(schemaTableName.getTableName())) { - return liveTable; + TableIdentifier remoteTable = toLiveRemoteTable(session, schemaTableName); + if (!remoteTable.name().equals(schemaTableName.getTableName())) { + return remoteTable; } - else if (!liveView.name().equals(schemaTableName.getTableName())) { - return liveView; + + TableIdentifier remoteView = toLiveRemoteView(session, schemaTableName); + if (!remoteView.name().equals(schemaTableName.getTableName())) { + return remoteView; } - else if (liveView.name().equals(schemaTableName.getTableName()) && liveTable.name().equals(schemaTableName.getTableName())) { - return liveTable; // table name and view name are same + else if (remoteView.name().equals(schemaTableName.getTableName()) && remoteTable.name().equals(schemaTableName.getTableName())) { + return remoteTable; // table name and view name are same } throw new RuntimeException("Unable to find remote object"); } @@ -515,7 +515,7 @@ public void renameView(ConnectorSession session, SchemaTableName source, SchemaT { restSessionCatalog.renameView(convert(session), toRemoteView(session, source), toRemoteView(session, target)); if (caseInsensitiveNameMatching) { - remoteViewMappingCache.invalidate(toIdentifier(source)); + remoteTableMappingCache.get().invalidate(toIdentifier(source)); } } @@ -530,7 +530,7 @@ public void dropView(ConnectorSession session, SchemaTableName schemaViewName) { restSessionCatalog.dropView(convert(session), toRemoteView(session, schemaViewName)); if (caseInsensitiveNameMatching) { - remoteViewMappingCache.invalidate(toIdentifier(schemaViewName)); + remoteTableMappingCache.get().invalidate(toIdentifier(schemaViewName)); } } @@ -584,10 +584,8 @@ public Optional getView(ConnectorSession session, Schem private Optional getIcebergView(ConnectorSession session, SchemaTableName viewName, boolean findLiveView) { try { - if (caseInsensitiveNameMatching) { - return Optional.of(restSessionCatalog.loadView(convert(session), findLiveView ? toLiveRemoteView(session, viewName) : toRemoteView(session, viewName))); - } - return Optional.of(restSessionCatalog.loadView(convert(session), toRemoteView(session, viewName))); + return Optional.of( + restSessionCatalog.loadView(convert(session), caseInsensitiveNameMatching && findLiveView ? toLiveRemoteView(session, viewName) : toRemoteView(session, viewName))); } catch (NoSuchViewException e) { return Optional.empty(); @@ -766,7 +764,7 @@ private TableIdentifier toRemoteTable(ConnectorSession session, SchemaTableName TableIdentifier tableIdentifier = toIdentifier(schemaTableName); if (caseInsensitiveNameMatching) { try { - return remoteTableMappingCache.get(tableIdentifier, () -> findRemoteTable(session, tableIdentifier)); + return remoteTableMappingCache.get().get(tableIdentifier, () -> findRemoteTable(session, tableIdentifier)); } catch (ExecutionException e) { throw new RuntimeException(e); @@ -806,7 +804,7 @@ private TableIdentifier toRemoteView(ConnectorSession session, SchemaTableName s TableIdentifier tableIdentifier = toIdentifier(schemaViewName); if (caseInsensitiveNameMatching) { try { - return remoteViewMappingCache.get(tableIdentifier, () -> findRemoteView(session, tableIdentifier)); + return remoteTableMappingCache.get().get(tableIdentifier, () -> findRemoteView(session, tableIdentifier)); } catch (ExecutionException e) { throw new RuntimeException(e); @@ -845,7 +843,7 @@ private Namespace toRemoteNamespace(ConnectorSession session, Namespace trinoNam { if (caseInsensitiveNameMatching) { try { - return remoteNamespaceMappingCache.get(trinoNamespace, () -> findRemoteNamespace(session, trinoNamespace)); + return remoteNamespaceMappingCache.get().get(trinoNamespace, () -> findRemoteNamespace(session, trinoNamespace)); } catch (ExecutionException e) { throw new RuntimeException(e); diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestTrinoRestCatalog.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestTrinoRestCatalog.java index 389cc43a7d555..e68d62ac64037 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestTrinoRestCatalog.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestTrinoRestCatalog.java @@ -16,7 +16,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import io.airlift.log.Logger; -import io.trino.cache.EvictableCacheBuilder; import io.trino.metastore.TableInfo; import io.trino.plugin.hive.NodeVersion; import io.trino.plugin.iceberg.CommitTaskData; @@ -54,7 +53,6 @@ import static io.trino.testing.TestingConnectorSession.SESSION; import static io.trino.testing.TestingNames.randomNameSuffix; import static java.util.Locale.ENGLISH; -import static java.util.concurrent.TimeUnit.MILLISECONDS; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -92,9 +90,8 @@ private static TrinoRestCatalog createTrinoRestCatalog(boolean useUniqueTableLoc new TestingTypeManager(), useUniqueTableLocations, false, - EvictableCacheBuilder.newBuilder().expireAfterWrite(1, MILLISECONDS).shareNothingWhenDisabled().build(), - EvictableCacheBuilder.newBuilder().expireAfterWrite(1, MILLISECONDS).shareNothingWhenDisabled().build(), - EvictableCacheBuilder.newBuilder().expireAfterWrite(1, MILLISECONDS).shareNothingWhenDisabled().build()); + Optional.empty(), + Optional.empty()); } @Test