From a7a72fd2c54fe166ce9d76eb8734ee580405a3d1 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Wed, 19 Jul 2023 17:02:45 +0200 Subject: [PATCH 1/6] Improve exception message when assertion fails --- .../src/test/java/io/trino/plugin/hive/AbstractTestHive.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java index 65a298c157bf..4bbcbb99f5df 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java @@ -2565,7 +2565,8 @@ public void testHiveViewsHaveNoColumns() { try (Transaction transaction = newTransaction()) { ConnectorMetadata metadata = transaction.getMetadata(); - assertEquals(listTableColumns(metadata, newSession(), new SchemaTablePrefix(view.getSchemaName(), view.getTableName())), ImmutableMap.of()); + assertThat(listTableColumns(metadata, newSession(), new SchemaTablePrefix(view.getSchemaName(), view.getTableName()))) + .isEmpty(); } } From a1b181caa5353eaeabd9f2a39969a13c94027769 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Wed, 19 Jul 2023 17:05:56 +0200 Subject: [PATCH 2/6] Move test method to subclass that has data set up The test was passing for `AbstractTestHiveLocal` because the test view was never created, so `streamTableColumns` couldn't return anything for it. --- .../test/java/io/trino/plugin/hive/TestHive.java | 13 +++++++++++++ .../java/io/trino/plugin/hive/AbstractTestHive.java | 12 +----------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/plugin/trino-hive-hadoop2/src/test/java/io/trino/plugin/hive/TestHive.java b/plugin/trino-hive-hadoop2/src/test/java/io/trino/plugin/hive/TestHive.java index f3fc938ca249..e55ffecf5059 100644 --- a/plugin/trino-hive-hadoop2/src/test/java/io/trino/plugin/hive/TestHive.java +++ b/plugin/trino-hive-hadoop2/src/test/java/io/trino/plugin/hive/TestHive.java @@ -15,13 +15,16 @@ import com.google.common.collect.ImmutableList; import com.google.common.net.HostAndPort; +import io.trino.spi.connector.ConnectorMetadata; import io.trino.spi.connector.SchemaTableName; +import io.trino.spi.connector.SchemaTablePrefix; import org.apache.hadoop.net.NetUtils; import org.testng.SkipException; import org.testng.annotations.BeforeClass; import org.testng.annotations.Parameters; import org.testng.annotations.Test; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; // staging directory is shared mutable state @@ -66,6 +69,16 @@ public void testHideDeltaLakeTables() throw new SkipException("not supported"); } + @Test + public void testHiveViewsHaveNoColumns() + { + try (Transaction transaction = newTransaction()) { + ConnectorMetadata metadata = transaction.getMetadata(); + assertThat(listTableColumns(metadata, newSession(), new SchemaTablePrefix(view.getSchemaName(), view.getTableName()))) + .isEmpty(); + } + } + @Test public void testHiveViewTranslationError() { diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java index 4bbcbb99f5df..c9b8b8a6573b 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java @@ -2560,16 +2560,6 @@ private void assertEmptyFile(HiveStorageFormat format) } } - @Test - public void testHiveViewsHaveNoColumns() - { - try (Transaction transaction = newTransaction()) { - ConnectorMetadata metadata = transaction.getMetadata(); - assertThat(listTableColumns(metadata, newSession(), new SchemaTablePrefix(view.getSchemaName(), view.getTableName()))) - .isEmpty(); - } - } - @Test public void testRenameTable() { @@ -3517,7 +3507,7 @@ public void testIllegalStorageFormatDuringTableScan() } } - private static Map> listTableColumns(ConnectorMetadata metadata, ConnectorSession session, SchemaTablePrefix prefix) + protected static Map> listTableColumns(ConnectorMetadata metadata, ConnectorSession session, SchemaTablePrefix prefix) { return stream(metadata.streamTableColumns(session, prefix)) .collect(toImmutableMap( From 5d257eca4ca5a5884a88a14bcb5224590e681c3c Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Thu, 20 Jul 2023 12:46:08 +0200 Subject: [PATCH 3/6] Remove unused method --- .../java/io/trino/plugin/hive/HiveBasicStatistics.java | 7 ------- .../java/io/trino/plugin/hive/PartitionStatistics.java | 5 ----- .../java/io/trino/plugin/hive/metastore/Partition.java | 7 ------- 3 files changed, 19 deletions(-) diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveBasicStatistics.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveBasicStatistics.java index bbf1c17844e6..0ae16566732c 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveBasicStatistics.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveBasicStatistics.java @@ -21,7 +21,6 @@ import java.util.OptionalLong; import static com.google.common.base.MoreObjects.toStringHelper; -import static com.google.common.base.Preconditions.checkArgument; import static java.util.Objects.requireNonNull; @Immutable @@ -84,12 +83,6 @@ public OptionalLong getOnDiskDataSizeInBytes() return onDiskDataSizeInBytes; } - public HiveBasicStatistics withAdjustedRowCount(long adjustment) - { - checkArgument(rowCount.isPresent(), "rowCount isn't present"); - return new HiveBasicStatistics(fileCount, OptionalLong.of(rowCount.getAsLong() + adjustment), inMemoryDataSizeInBytes, onDiskDataSizeInBytes); - } - @Override public boolean equals(Object o) { diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/PartitionStatistics.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/PartitionStatistics.java index 40511ee31181..3537517abe6a 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/PartitionStatistics.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/PartitionStatistics.java @@ -60,11 +60,6 @@ public Map getColumnStatistics() return columnStatistics; } - public PartitionStatistics withAdjustedRowCount(long adjustment) - { - return new PartitionStatistics(basicStatistics.withAdjustedRowCount(adjustment), columnStatistics); - } - @Override public boolean equals(Object o) { diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/Partition.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/Partition.java index 5d6667d481c7..19e3e02bf80e 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/Partition.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/Partition.java @@ -27,7 +27,6 @@ import java.util.function.Consumer; import static com.google.common.base.MoreObjects.toStringHelper; -import static io.trino.plugin.hive.metastore.MetastoreUtil.adjustRowCount; import static java.util.Objects.requireNonNull; @Immutable @@ -57,12 +56,6 @@ public Partition( this.parameters = ImmutableMap.copyOf(requireNonNull(parameters, "parameters is null")); } - @JsonIgnore - public Partition withAdjustedRowCount(String partitionName, long rowCountDelta) - { - return new Partition(databaseName, tableName, values, storage, columns, adjustRowCount(parameters, partitionName, rowCountDelta)); - } - @JsonProperty public String getDatabaseName() { From 5cd060fc10e270c49c54b2ee17b8cf8974f52944 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Thu, 20 Jul 2023 12:36:22 +0200 Subject: [PATCH 4/6] Refactor BaseHiveConnectorTest not to assume file metastore with certain path It's up to a subclass to configure connector in some way. --- .../main/java/io/trino/plugin/hive/HiveConnector.java | 9 +++++++++ .../plugin/hive/InternalHiveConnectorFactory.java | 1 + .../io/trino/plugin/hive/BaseHiveConnectorTest.java | 10 +++++++--- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConnector.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConnector.java index e52f5855100f..862c380f0a40 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConnector.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConnector.java @@ -15,6 +15,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.inject.Injector; import io.airlift.bootstrap.LifeCycleManager; import io.trino.plugin.base.classloader.ClassLoaderSafeConnectorMetadata; import io.trino.plugin.base.session.SessionPropertiesProvider; @@ -46,6 +47,7 @@ public class HiveConnector implements Connector { + private final Injector injector; private final LifeCycleManager lifeCycleManager; private final ConnectorSplitManager splitManager; private final ConnectorPageSourceProvider pageSourceProvider; @@ -68,6 +70,7 @@ public class HiveConnector private final boolean singleStatementWritesOnly; public HiveConnector( + Injector injector, LifeCycleManager lifeCycleManager, HiveTransactionManager transactionManager, ConnectorSplitManager splitManager, @@ -87,6 +90,7 @@ public HiveConnector( boolean singleStatementWritesOnly, ClassLoader classLoader) { + this.injector = requireNonNull(injector, "injector is null"); this.lifeCycleManager = requireNonNull(lifeCycleManager, "lifeCycleManager is null"); this.transactionManager = requireNonNull(transactionManager, "transactionManager is null"); this.splitManager = requireNonNull(splitManager, "splitManager is null"); @@ -233,4 +237,9 @@ public Set getTableProcedures() { return tableProcedures; } + + public Injector getInjector() + { + return injector; + } } diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/InternalHiveConnectorFactory.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/InternalHiveConnectorFactory.java index b954aaea043b..be50bb4961d9 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/InternalHiveConnectorFactory.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/InternalHiveConnectorFactory.java @@ -165,6 +165,7 @@ public static Connector createConnector( .map(accessControl -> new ClassLoaderSafeConnectorAccessControl(accessControl, classLoader)); return new HiveConnector( + injector, lifeCycleManager, transactionManager, new ClassLoaderSafeConnectorSplitManager(splitManager, classLoader), diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java index 61eef9341ec4..a0c3ec8e0ec9 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java @@ -30,11 +30,12 @@ import io.trino.metadata.TableHandle; import io.trino.metadata.TableMetadata; import io.trino.plugin.hive.metastore.Column; +import io.trino.plugin.hive.metastore.HiveMetastore; +import io.trino.plugin.hive.metastore.HiveMetastoreFactory; import io.trino.plugin.hive.metastore.PrincipalPrivileges; import io.trino.plugin.hive.metastore.Storage; import io.trino.plugin.hive.metastore.StorageFormat; import io.trino.plugin.hive.metastore.Table; -import io.trino.plugin.hive.metastore.file.FileHiveMetastore; import io.trino.spi.connector.CatalogSchemaTableName; import io.trino.spi.connector.ColumnHandle; import io.trino.spi.connector.ColumnMetadata; @@ -143,7 +144,6 @@ import static io.trino.plugin.hive.HiveTableProperties.PARTITIONED_BY_PROPERTY; import static io.trino.plugin.hive.HiveTableProperties.STORAGE_FORMAT_PROPERTY; import static io.trino.plugin.hive.HiveType.toHiveType; -import static io.trino.plugin.hive.metastore.file.TestingFileHiveMetastore.createTestingFileHiveMetastore; import static io.trino.plugin.hive.util.HiveUtil.columnExtraInfo; import static io.trino.spi.security.Identity.ofUser; import static io.trino.spi.security.SelectedRole.Type.ROLE; @@ -8370,6 +8370,8 @@ public void testSelectFromPrestoViewReferencingHiveTableWithTimestamps() @Test public void testTimestampWithTimeZone() { + String catalog = getSession().getCatalog().orElseThrow(); + assertUpdate("CREATE TABLE test_timestamptz_base (t timestamp) WITH (format = 'PARQUET')"); assertUpdate("INSERT INTO test_timestamptz_base (t) VALUES" + "(timestamp '2022-07-26 12:13')", 1); @@ -8379,7 +8381,9 @@ public void testTimestampWithTimeZone() String tableLocation = getTableLocation("test_timestamptz_base"); // TIMESTAMP WITH LOCAL TIME ZONE is not mapped to any Trino type, so we need to create the metastore entry manually - FileHiveMetastore metastore = createTestingFileHiveMetastore(new File(getDistributedQueryRunner().getCoordinator().getBaseDataDir().toFile(), "hive_data")); + HiveMetastore metastore = ((HiveConnector) getDistributedQueryRunner().getCoordinator().getConnector(catalog)) + .getInjector().getInstance(HiveMetastoreFactory.class) + .createMetastore(Optional.of(getSession().getIdentity().toConnectorIdentity(catalog))); metastore.createTable( new Table( "tpch", From b39196b4136ce1349c9b416a207396fda59adf58 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Thu, 20 Jul 2023 13:37:55 +0200 Subject: [PATCH 5/6] Pin takari-lifecycle-plugin version --- core/trino-server/pom.xml | 1 + pom.xml | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/core/trino-server/pom.xml b/core/trino-server/pom.xml index 78dec8600c21..69d361f36dba 100644 --- a/core/trino-server/pom.xml +++ b/core/trino-server/pom.xml @@ -53,6 +53,7 @@ io.takari.maven.plugins takari-lifecycle-plugin + ${dep.takari.version} none diff --git a/pom.xml b/pom.xml index bd1a6fe2be38..068a1231a97e 100644 --- a/pom.xml +++ b/pom.xml @@ -155,6 +155,7 @@ 12.0.1 1.11.1 ${dep.airlift.version} + 2.1.1 1.12.505 2.20.93 0.11.5 @@ -2211,7 +2212,7 @@ io.takari.maven.plugins takari-lifecycle-plugin - 2.1.1 + ${dep.takari.version} PLUGIN From e4ecb162c1c2907b80c56b6597cc96f4fe6449bd Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Wed, 19 Jul 2023 13:55:52 +0200 Subject: [PATCH 6/6] Fix HiveMetadata.streamTableColumns not to include views In the absence of explicit documentation, the use place (`io.trino.metadata.MetadataManager#listTableColumns`) defines `streamTableColumns` contract as covering tables only. This commit also supplements explicit documentation in `ConnectorMetadata`. This relates to a logic introduced in `HiveMetadata.doGetTableMetadata` in aafe4797c34ecad00415691252e1a83b4207efba commit. It was practical for hive views and when view translation is not enabled, but was not correct for trino views. --- .../spi/connector/ConnectorMetadata.java | 4 ++-- .../io/trino/plugin/hive/HiveMetadata.java | 21 ++++++++++++++++++- .../tests/product/hive/TestHiveViews.java | 4 ++-- 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java b/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java index 24ee28c83439..0968e8af6f7d 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java +++ b/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java @@ -286,7 +286,7 @@ default ColumnMetadata getColumnMetadata(ConnectorSession session, ConnectorTabl } /** - * Gets the metadata for all columns that match the specified table prefix. + * Gets the metadata for all columns that match the specified table prefix. Columns of views and materialized views are not included. * * @deprecated use {@link #streamTableColumns} which handles redirected tables */ @@ -298,7 +298,7 @@ default Map> listTableColumns(ConnectorSes /** * Gets the metadata for all columns that match the specified table prefix. Redirected table names are included, but - * the column metadata for them is not. + * the column metadata for them is not. Views and materialized views are not included. */ default Iterator streamTableColumns(ConnectorSession session, SchemaTablePrefix prefix) { diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java index c4aaf8c97f16..f4c20c95dcfc 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java @@ -257,6 +257,7 @@ import static io.trino.plugin.hive.ViewReaderUtil.encodeViewData; import static io.trino.plugin.hive.ViewReaderUtil.isHiveOrPrestoView; import static io.trino.plugin.hive.ViewReaderUtil.isPrestoView; +import static io.trino.plugin.hive.ViewReaderUtil.isTrinoMaterializedView; import static io.trino.plugin.hive.acid.AcidTransaction.NO_ACID_TRANSACTION; import static io.trino.plugin.hive.acid.AcidTransaction.forCreateTable; import static io.trino.plugin.hive.metastore.MetastoreUtil.buildInitialPrivilegeSet; @@ -614,7 +615,24 @@ private ConnectorTableMetadata doGetTableMetadata(ConnectorSession session, Sche throw new TrinoException(UNSUPPORTED_TABLE_TYPE, format("Not a Hive table '%s'", tableName)); } - if (!translateHiveViews && isHiveOrPrestoView(table)) { + boolean isTrinoView = isPrestoView(table); + boolean isHiveView = !isTrinoView && isHiveOrPrestoView(table); + boolean isTrinoMaterializedView = isTrinoMaterializedView(table); + if (isHiveView && translateHiveViews) { + // Produce metadata for a (translated) Hive view as if it was a table. This is incorrect from ConnectorMetadata.streamTableColumns + // perspective, but is done on purpose to keep information_schema.columns working. + // Because of fallback in ThriftHiveMetastoreClient.getAllViews, this method may return Trino/Presto views only, + // so HiveMetadata.getViews may fail to return Hive views. + } + else if (isHiveView) { + // When Hive view translation is not enabled, a Hive view is currently treated inconsistently + // - getView treats this as an unusable view (fails instead of returning Optional.empty) + // - getTableHandle treats this as a table (returns non-null) + // In any case, returning metadata is not useful. + throw new TableNotFoundException(tableName); + } + else if (isTrinoView || isTrinoMaterializedView) { + // streamTableColumns should not include views and materialized views throw new TableNotFoundException(tableName); } @@ -837,6 +855,7 @@ private Stream streamTableColumns(ConnectorSession session return Stream.empty(); } catch (TableNotFoundException e) { + // it is not a table (e.g. it's a view) (TODO remove exception-driven logic for this case) OR // table disappeared during listing operation return Stream.empty(); } diff --git a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveViews.java b/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveViews.java index 08f1caaba403..a29d50aea837 100644 --- a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveViews.java +++ b/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveViews.java @@ -91,7 +91,7 @@ private void testFailingHiveViewsWithInformationSchema() .hasMessageContaining("Failed to translate Hive view 'test_list_failing_views.failing_view'"); // Queries on information_schema.columns also trigger ConnectorMetadata#getViews. Columns from failing_view are - // listed too since HiveMetadata#listTableColumns does not ignore views. + // listed too since HiveMetadata#listTableColumns does not ignore Hive views. assertThat(onTrino().executeQuery("SELECT table_name, column_name FROM information_schema.columns WHERE table_schema = 'test_list_failing_views'")) .containsOnly( row("correct_view", "n_nationkey"), @@ -141,7 +141,7 @@ private void testFailingHiveViewsWithSystemJdbc() .hasMessageContaining("Failed to translate Hive view 'test_list_failing_views.failing_view'"); // Queries on system.jdbc.columns also trigger ConnectorMetadata#getViews. Columns from failing_view are - // listed too since HiveMetadata#listTableColumns does not ignore views. + // listed too since HiveMetadata#listTableColumns does not ignore Hive views. assertThat(onTrino().executeQuery("SELECT table_name, column_name FROM system.jdbc.columns WHERE table_cat = 'hive' AND table_schem = 'test_list_failing_views'")) .containsOnly( row("correct_view", "n_nationkey"),