From e2ca2c64a64d387344e7cfe48dd56c27a5e2fb72 Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Sat, 19 Feb 2022 16:38:57 +0900 Subject: [PATCH 1/5] Don't insert rows in skipTestUnlessSupportsDeletes An empty table is sufficient to verify the connector behavior. --- .../java/io/trino/testing/AbstractTestDistributedQueries.java | 2 +- .../src/main/java/io/trino/testing/BaseConnectorTest.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java b/testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java index e2a4a419aa18..c21f36f1ad55 100644 --- a/testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java +++ b/testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java @@ -771,7 +771,7 @@ public void testDeleteWithVarcharPredicate() protected void skipTestUnlessSupportsDeletes() { skipTestUnless(supportsCreateTable()); - try (TestTable table = new TestTable(getQueryRunner()::execute, "test_supports_delete", "(col varchar(1))", ImmutableList.of("'a'", "'A'"))) { + try (TestTable table = new TestTable(getQueryRunner()::execute, "test_supports_delete", "(col varchar(1))")) { if (!supportsDelete()) { assertQueryFails("DELETE FROM " + table.getName(), "This connector does not support deletes"); throw new SkipException("This connector does not support deletes"); diff --git a/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java b/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java index c7ac5b5cf7f7..5c42f1868b07 100644 --- a/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java +++ b/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java @@ -1296,7 +1296,7 @@ public void verifySupportsDeleteDeclaration() } skipTestUnless(hasBehavior(SUPPORTS_CREATE_TABLE)); - try (TestTable table = new TestTable(getQueryRunner()::execute, "test_supports_delete", "AS SELECT * FROM region")) { + try (TestTable table = new TestTable(getQueryRunner()::execute, "test_supports_delete", "(regionkey int)")) { assertQueryFails("DELETE FROM " + table.getName(), "This connector does not support deletes"); } } @@ -1310,7 +1310,7 @@ public void verifySupportsRowLevelDeleteDeclaration() } skipTestUnless(hasBehavior(SUPPORTS_CREATE_TABLE)); - try (TestTable table = new TestTable(getQueryRunner()::execute, "test_supports_row_level_delete", "AS SELECT * FROM region")) { + try (TestTable table = new TestTable(getQueryRunner()::execute, "test_supports_row_level_delete", "(regionkey int)")) { assertQueryFails("DELETE FROM " + table.getName() + " WHERE regionkey = 2", "This connector does not support deletes"); } } From 13690df32c5b67215be9bac0b8c2a0403ae72766 Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Sat, 19 Feb 2022 17:03:08 +0900 Subject: [PATCH 2/5] Create an empty table in testQueryLoggingCount - Allow overriding the table definition - Set table properties in Kudu to provide table definition --- .../plugin/kudu/AbstractKuduConnectorTest.java | 13 +++++++++++++ .../testing/AbstractTestDistributedQueries.java | 13 +++++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/AbstractKuduConnectorTest.java b/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/AbstractKuduConnectorTest.java index 2f1715227db1..49ee3fa8cfb7 100644 --- a/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/AbstractKuduConnectorTest.java +++ b/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/AbstractKuduConnectorTest.java @@ -432,6 +432,19 @@ protected TestTable createTableWithDefaultColumns() throw new SkipException("Kudu connector does not support column default values"); } + @Override + protected String tableDefinitionForQueryLoggingCount() + { + return "( " + + " foo_1 int WITH (primary_key=true), " + + " foo_2_4 int " + + ") " + + "WITH ( " + + " partition_by_hash_columns = ARRAY['foo_1'], " + + " partition_by_hash_buckets = 2 " + + ")"; + } + private void assertTableProperty(String tableProperties, String key, String regexValue) { assertTrue(Pattern.compile(key + "\\s*=\\s*" + regexValue + ",?\\s+").matcher(tableProperties).find(), diff --git a/testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java b/testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java index c21f36f1ad55..52d54a1e8404 100644 --- a/testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java +++ b/testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java @@ -1035,8 +1035,8 @@ public void testQueryLoggingCount() long beforeCompletedQueriesCount = waitUntilStable(() -> dispatchManager.getStats().getCompletedQueries().getTotalCount(), new Duration(5, SECONDS)); long beforeSubmittedQueriesCount = dispatchManager.getStats().getSubmittedQueries().getTotalCount(); String tableName = "test_logging_count" + randomTableSuffix(); - assertUpdate("CREATE TABLE " + tableName + " AS SELECT 1 foo_1, 2 foo_2_4", 1); - assertQuery("SELECT foo_1, foo_2_4 FROM " + tableName, "SELECT 1, 2"); + assertUpdate("CREATE TABLE " + tableName + tableDefinitionForQueryLoggingCount()); + assertQueryReturnsEmptyResult("SELECT foo_1, foo_2_4 FROM " + tableName); assertUpdate("DROP TABLE " + tableName); assertQueryFails("SELECT * FROM " + tableName, ".*Table .* does not exist"); @@ -1048,6 +1048,15 @@ public void testQueryLoggingCount() }); } + /** + * The table must have two columns foo_1 and foo_2_4 of any type. + */ + @Language("SQL") + protected String tableDefinitionForQueryLoggingCount() + { + return "(foo_1 int, foo_2_4 int)"; + } + private T waitUntilStable(Supplier computation, Duration timeout) { T lastValue = computation.get(); From 8b9ce6795f2430649ddfae44ce05b8810b4be14f Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Fri, 25 Feb 2022 09:02:15 +0900 Subject: [PATCH 3/5] Make AbstractTestDistributedQueries.testColumnName protected --- .../trino/plugin/kudu/AbstractKuduConnectorTest.java | 11 +---------- .../trino/testing/AbstractTestDistributedQueries.java | 2 +- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/AbstractKuduConnectorTest.java b/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/AbstractKuduConnectorTest.java index 49ee3fa8cfb7..5a4dba6a74a8 100644 --- a/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/AbstractKuduConnectorTest.java +++ b/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/AbstractKuduConnectorTest.java @@ -189,17 +189,8 @@ public void testRowDelete() assertUpdate("DROP TABLE test_row_delete"); } - @Test(dataProvider = "testColumnNameDataProvider") @Override - public void testColumnName(String columnName) - { - if (!requiresDelimiting(columnName)) { - testColumnName(columnName, false); - } - testColumnName(columnName, true); - } - - private void testColumnName(String columnName, boolean delimited) + protected void testColumnName(String columnName, boolean delimited) { String nameInSql = columnName; if (delimited) { diff --git a/testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java b/testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java index 52d54a1e8404..f090aee7f5ac 100644 --- a/testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java +++ b/testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java @@ -1206,7 +1206,7 @@ public void testColumnName(String columnName) testColumnName(columnName, true); } - private void testColumnName(String columnName, boolean delimited) + protected void testColumnName(String columnName, boolean delimited) { String nameInSql = columnName; if (delimited) { From 764c566cdc460688226124062f00c99cf4a779b7 Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Sat, 19 Feb 2022 17:09:12 +0900 Subject: [PATCH 4/5] Rename TestBigQueryIntegrationSmokeTest to TestBigQueryConnectorTest This is a preparatory commit for migrating to BaseConnectorTest. --- plugin/trino-bigquery/pom.xml | 4 ++-- ...tegrationSmokeTest.java => TestBigQueryConnectorTest.java} | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) rename plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/{TestBigQueryIntegrationSmokeTest.java => TestBigQueryConnectorTest.java} (99%) diff --git a/plugin/trino-bigquery/pom.xml b/plugin/trino-bigquery/pom.xml index 102491065e93..0a4df62006e6 100644 --- a/plugin/trino-bigquery/pom.xml +++ b/plugin/trino-bigquery/pom.xml @@ -351,7 +351,7 @@ - **/TestBigQueryIntegrationSmokeTest.java + **/TestBigQueryConnectorTest.java **/TestBigQueryTypeMapping.java **/TestBigQueryMetadata.java **/TestBigQueryInstanceCleaner.java @@ -375,7 +375,7 @@ maven-surefire-plugin - **/TestBigQueryIntegrationSmokeTest.java + **/TestBigQueryConnectorTest.java **/TestBigQueryTypeMapping.java **/TestBigQueryMetadata.java **/TestBigQueryInstanceCleaner.java diff --git a/plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryIntegrationSmokeTest.java b/plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryConnectorTest.java similarity index 99% rename from plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryIntegrationSmokeTest.java rename to plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryConnectorTest.java index 1ed42af3429e..a277eeb1450a 100644 --- a/plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryIntegrationSmokeTest.java +++ b/plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryConnectorTest.java @@ -35,7 +35,7 @@ import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertTrue; -public class TestBigQueryIntegrationSmokeTest +public class TestBigQueryConnectorTest // TODO extend BaseConnectorTest extends AbstractTestIntegrationSmokeTest { From d6b01599740b29da035ae8438d1e17890909922b Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Sat, 19 Feb 2022 17:10:12 +0900 Subject: [PATCH 5/5] Migrate to BaseConnectorTest in BigQuery --- .../bigquery/TestBigQueryConnectorTest.java | 206 +++++++++++++++++- 1 file changed, 203 insertions(+), 3 deletions(-) diff --git a/plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryConnectorTest.java b/plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryConnectorTest.java index a277eeb1450a..cecd634b0761 100644 --- a/plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryConnectorTest.java +++ b/plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryConnectorTest.java @@ -13,10 +13,12 @@ */ package io.trino.plugin.bigquery; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import io.trino.testing.AbstractTestIntegrationSmokeTest; +import io.trino.testing.BaseConnectorTest; import io.trino.testing.MaterializedResult; import io.trino.testing.QueryRunner; +import io.trino.testing.TestingConnectorBehavior; import io.trino.testing.sql.TestTable; import io.trino.testing.sql.TestView; import org.testng.annotations.BeforeClass; @@ -25,19 +27,21 @@ import java.util.List; +import static com.google.common.base.Strings.nullToEmpty; import static io.trino.plugin.bigquery.BigQueryQueryRunner.BigQuerySqlExecutor; import static io.trino.spi.type.VarcharType.VARCHAR; import static io.trino.testing.MaterializedResult.resultBuilder; import static io.trino.testing.assertions.Assert.assertEquals; import static io.trino.testing.sql.TestTable.randomTableSuffix; import static java.lang.String.format; +import static java.util.Locale.ENGLISH; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertTrue; public class TestBigQueryConnectorTest - // TODO extend BaseConnectorTest - extends AbstractTestIntegrationSmokeTest + extends BaseConnectorTest { protected BigQuerySqlExecutor bigQuerySqlExecutor; @@ -56,7 +60,30 @@ protected QueryRunner createQueryRunner() ImmutableMap.of()); } + @Override + protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) + { + switch (connectorBehavior) { + case SUPPORTS_TOPN_PUSHDOWN: + case SUPPORTS_RENAME_SCHEMA: + case SUPPORTS_RENAME_TABLE: + case SUPPORTS_NOT_NULL_CONSTRAINT: + case SUPPORTS_CREATE_TABLE_WITH_DATA: + case SUPPORTS_DELETE: + case SUPPORTS_INSERT: + case SUPPORTS_ADD_COLUMN: + case SUPPORTS_DROP_COLUMN: + case SUPPORTS_RENAME_COLUMN: + case SUPPORTS_COMMENT_ON_TABLE: + case SUPPORTS_COMMENT_ON_COLUMN: + return false; + default: + return super.hasBehavior(connectorBehavior); + } + } + @Test + @Override public void testCreateSchema() { String schemaName = "test_schema_create_" + randomTableSuffix(); @@ -82,6 +109,28 @@ public void testCreateSchema() assertUpdate("DROP SCHEMA IF EXISTS " + schemaName); } + @Test + @Override + public void testShowColumns() + { + // shippriority column is bigint (not integer) in BigQuery connector + MaterializedResult actual = computeActual("SHOW COLUMNS FROM orders"); + + MaterializedResult expectedParametrizedVarchar = resultBuilder(getSession(), VARCHAR, VARCHAR, VARCHAR, VARCHAR) + .row("orderkey", "bigint", "", "") + .row("custkey", "bigint", "", "") + .row("orderstatus", "varchar", "", "") + .row("totalprice", "double", "", "") + .row("orderdate", "date", "", "") + .row("orderpriority", "varchar", "", "") + .row("clerk", "varchar", "", "") + .row("shippriority", "bigint", "", "") + .row("comment", "varchar", "", "") + .build(); + + assertEquals(actual, expectedParametrizedVarchar); + } + @Override public void testDescribeTable() { @@ -181,6 +230,22 @@ public void testCreateTableIfNotExists() } } + @Test + @Override + public void testCreateTableAsSelect() + { + assertThatThrownBy(super::testCreateTableAsSelect) + .hasStackTraceContaining("This connector does not support creating tables with data"); + } + + @Test + @Override + public void testCreateTableAsSelectWithUnicode() + { + assertThatThrownBy(super::testCreateTableAsSelectWithUnicode) + .hasStackTraceContaining("This connector does not support creating tables with data"); + } + @Test public void testDropTable() { @@ -192,6 +257,80 @@ public void testDropTable() assertFalse(getQueryRunner().tableExists(getSession(), tableName)); } + @Test + @Override + public void testRenameTable() + { + // Use CREATE TABLE instead of CREATE TABLE AS statement + String tableName = "test_rename_" + randomTableSuffix(); + assertUpdate("CREATE TABLE " + tableName + " (x int)"); + + String renamedTable = "test_rename_new_" + randomTableSuffix(); + assertQueryFails("ALTER TABLE " + tableName + " RENAME TO " + renamedTable, "This connector does not support renaming tables"); + } + + @Test(dataProvider = "testDataMappingSmokeTestDataProvider") + @Override + public void testDataMappingSmokeTest(DataMappingTestSetup dataMappingTestSetup) + { + assertThatThrownBy(() -> super.testDataMappingSmokeTest(dataMappingTestSetup)) + .hasMessageContaining("This connector does not support creating tables with data"); + } + + @Test(dataProvider = "testCaseSensitiveDataMappingProvider") + @Override + public void testCaseSensitiveDataMapping(DataMappingTestSetup dataMappingTestSetup) + { + assertThatThrownBy(() -> super.testCaseSensitiveDataMapping(dataMappingTestSetup)) + .hasMessageContaining("This connector does not support creating tables with data"); + } + + @Override + protected void testColumnName(String columnName, boolean delimited) + { + // Override because BigQuery connector doesn't support INSERT statement + String nameInSql = columnName; + if (delimited) { + nameInSql = "\"" + columnName.replace("\"", "\"\"") + "\""; + } + String tableName = "test.tcn_" + nameInSql.toLowerCase(ENGLISH).replaceAll("[^a-z0-9]", "") + randomTableSuffix(); + + try { + // TODO test with both CTAS *and* CREATE TABLE + INSERT, since they use different connector API methods. + assertUpdate("CREATE TABLE " + tableName + "(key varchar(50), " + nameInSql + " varchar(50))"); + } + catch (RuntimeException e) { + if (isColumnNameRejected(e, columnName, delimited)) { + // It is OK if give column name is not allowed and is clearly rejected by the connector. + return; + } + throw e; + } + try { + // Execute INSERT statement in BigQuery + onBigQuery("INSERT INTO " + tableName + " VALUES ('null value', NULL), ('sample value', 'abc'), ('other value', 'xyz')"); + + // SELECT * + assertQuery("SELECT * FROM " + tableName, "VALUES ('null value', NULL), ('sample value', 'abc'), ('other value', 'xyz')"); + + // projection + assertQuery("SELECT " + nameInSql + " FROM " + tableName, "VALUES (NULL), ('abc'), ('xyz')"); + + // predicate + assertQuery("SELECT key FROM " + tableName + " WHERE " + nameInSql + " IS NULL", "VALUES ('null value')"); + assertQuery("SELECT key FROM " + tableName + " WHERE " + nameInSql + " = 'abc'", "VALUES ('sample value')"); + } + finally { + assertUpdate("DROP TABLE " + tableName); + } + } + + @Override + protected boolean isColumnNameRejected(Exception exception, String columnName, boolean delimited) + { + return nullToEmpty(exception.getMessage()).matches(".*(Fields must contain only letters, numbers, and underscores, start with a letter or underscore, and be at most 300 characters long).*"); + } + @Test(enabled = false) public void testSelectFromHourlyPartitionedTable() { @@ -323,6 +462,46 @@ public void testShowCreateTable() ")"); } + @Test + @Override + public void testCharVarcharComparison() + { + // BigQuery doesn't have char type + assertThatThrownBy(super::testCharVarcharComparison) + .hasMessage("This connector does not support creating tables with data"); + } + + @Test + @Override + public void testVarcharCharComparison() + { + // Use BigQuery SQL executor because the connector doesn't support INSERT statement + try (TestTable table = new TestTable( + bigQuerySqlExecutor, + "test.test_varchar_char", + "(k int, v string(3))", + ImmutableList.of( + "-1, NULL", + "0, ''", + "1, ' '", + "2, ' '", + "3, ' '", + "4, 'x'", + "5, 'x '", + "6, 'x '"))) { + assertQuery( + "SELECT k, v FROM " + table.getName() + " WHERE v = CAST(' ' AS char(2))", + // The 3-spaces value is included because both sides of the comparison are coerced to char(3) + "VALUES (0, ''), (1, ' '), (2, ' '), (3, ' ')"); + + // value that's not all-spaces + assertQuery( + "SELECT k, v FROM " + table.getName() + " WHERE v = CAST('x ' AS char(2))", + // The 3-spaces value is included because both sides of the comparison are coerced to char(3) + "VALUES (4, 'x'), (5, 'x '), (6, 'x ')"); + } + } + @Test public void testSkipUnsupportedType() { @@ -340,6 +519,27 @@ public void testSkipUnsupportedType() } } + @Test + @Override + public void testDateYearOfEraPredicate() + { + // Override because the connector throws an exception instead of an empty result when the value is out of supported range + assertQuery("SELECT orderdate FROM orders WHERE orderdate = DATE '1997-09-14'", "VALUES DATE '1997-09-14'"); + assertThatThrownBy(() -> query("SELECT * FROM orders WHERE orderdate = DATE '-1996-09-14'")) + .hasMessageMatching(".*Row filter for .* is invalid\\. Filter is '\\(`orderdate` = '-1996-09-14'\\)'"); + } + + @Test + @Override + public void testSymbolAliasing() + { + // Create table in BigQuery because the connector doesn't support CREATE TABLE AS SELECT statement + String tableName = "test.test_symbol_aliasing" + randomTableSuffix(); + onBigQuery("CREATE TABLE " + tableName + " AS SELECT 1 foo_1, 2 foo_2_4"); + assertQuery("SELECT foo_1, foo_2_4 FROM " + tableName, "SELECT 1, 2"); + assertUpdate("DROP TABLE " + tableName); + } + private void onBigQuery(String sql) { bigQuerySqlExecutor.execute(sql);