Update according to reviews 11/01/2024

bloomberg · Jan 11, 2024 · a185e04 · a185e04
1 parent 69d4ab2
commit a185e04
Show file tree

Hide file tree

Showing 10 changed files with 37 additions and 161 deletions.
diff --git a/docs/src/main/sphinx/connector/snowflake.md b/docs/src/main/sphinx/connector/snowflake.md
@@ -71,8 +71,6 @@ Trino supports the following Snowflake data types:
 | `date`         | `date`         |
 | `time`         | `time`         |
 | `timestampntz` | `timestamp`    |
-| `timestamptz`  | `timestampTZ`  |
-| `timestampltz` | `timestampTZ`  |
 
 Complete list of [Snowflake data types](https://docs.snowflake.com/en/sql-reference/intro-summary-data-types.html).
 

diff --git a/plugin/trino-snowflake/pom.xml b/plugin/trino-snowflake/pom.xml
@@ -5,7 +5,7 @@
     <parent>
         <groupId>io.trino</groupId>
         <artifactId>trino-root</artifactId>
-        <version>435-SNAPSHOT</version>
+        <version>436-SNAPSHOT</version>
         <relativePath>../../pom.xml</relativePath>
     </parent>
 
@@ -93,6 +93,12 @@
         </dependency>
 
         <!-- for testing -->
+        <dependency>
+            <groupId>io.airlift</groupId>
+            <artifactId>junit-extensions</artifactId>
+            <scope>test</scope>
+        </dependency>
+
         <dependency>
             <groupId>io.airlift</groupId>
             <artifactId>testing</artifactId>

diff --git a/plugin/trino-snowflake/src/main/java/io/trino/plugin/snowflake/SnowflakeClient.java b/plugin/trino-snowflake/src/main/java/io/trino/plugin/snowflake/SnowflakeClient.java
diff --git a/plugin/trino-snowflake/src/main/java/io/trino/plugin/snowflake/SnowflakeClientModule.java b/plugin/trino-snowflake/src/main/java/io/trino/plugin/snowflake/SnowflakeClientModule.java
@@ -18,6 +18,7 @@
 import com.google.inject.Provides;
 import com.google.inject.Scopes;
 import com.google.inject.Singleton;
+import io.opentelemetry.api.OpenTelemetry;
 import io.trino.plugin.jdbc.BaseJdbcConfig;
 import io.trino.plugin.jdbc.ConnectionFactory;
 import io.trino.plugin.jdbc.DriverConnectionFactory;
@@ -49,7 +50,7 @@ public void configure(Binder binder)
     @Singleton
     @Provides
     @ForBaseJdbc
-    public ConnectionFactory getConnectionFactory(BaseJdbcConfig baseJdbcConfig, SnowflakeConfig snowflakeConfig, CredentialProvider credentialProvider)
+    public ConnectionFactory getConnectionFactory(BaseJdbcConfig baseJdbcConfig, SnowflakeConfig snowflakeConfig, CredentialProvider credentialProvider, OpenTelemetry openTelemetry)
             throws MalformedURLException
     {
         Properties properties = new Properties();
@@ -90,6 +91,6 @@ public ConnectionFactory getConnectionFactory(BaseJdbcConfig baseJdbcConfig, Sno
             }
         }
 
-        return new DriverConnectionFactory(new SnowflakeDriver(), baseJdbcConfig.getConnectionUrl(), properties, credentialProvider);
+        return new DriverConnectionFactory(new SnowflakeDriver(), baseJdbcConfig.getConnectionUrl(), properties, credentialProvider, openTelemetry);
     }
 }
diff --git a/plugin/trino-snowflake/src/main/java/io/trino/plugin/snowflake/SnowflakeConfig.java b/plugin/trino-snowflake/src/main/java/io/trino/plugin/snowflake/SnowflakeConfig.java
@@ -79,13 +79,6 @@ public Optional<Boolean> getTimestampNoTimezoneAsUTC()
         return Optional.ofNullable(timestampNoTimezoneAsUTC);
     }
 
-    @Config("snowflake.timestamp-no-timezone-as-utc")
-    public SnowflakeConfig setTimestampNoTimezoneAsUTC(Boolean timestampNoTimezoneAsUTC)
-    {
-        this.timestampNoTimezoneAsUTC = timestampNoTimezoneAsUTC;
-        return this;
-    }
-
     public Optional<String> getHTTPProxy()
     {
         return Optional.ofNullable(httpProxy);

diff --git a/...n/trino-snowflake/src/test/java/io/trino/plugin/snowflake/BaseSnowflakeConnectorTest.java b/...n/trino-snowflake/src/test/java/io/trino/plugin/snowflake/BaseSnowflakeConnectorTest.java
@@ -59,12 +59,10 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
                 return false;
             case SUPPORTS_COMMENT_ON_COLUMN:
             case SUPPORTS_ADD_COLUMN_WITH_COMMENT:
-            case SUPPORTS_COMMENT_ON_TABLE:
             case SUPPORTS_CREATE_TABLE_WITH_TABLE_COMMENT:
             case SUPPORTS_CREATE_TABLE_WITH_COLUMN_COMMENT:
             case SUPPORTS_SET_COLUMN_TYPE:
                 return false;
-            case SUPPORTS_DROP_FIELD:
             case SUPPORTS_ROW_TYPE:
             case SUPPORTS_ARRAY:
                 return false;
@@ -323,7 +321,6 @@ public void testCreateTableAsSelect()
                         "SELECT 1234567890, 123",
                 "SELECT count(*) + 1 FROM nation");
 
-        // TODO: BigQuery throws table not found at BigQueryClient.insert if we reuse the same table name
         tableName = "test_ctas" + randomNameSuffix();
         assertExplainAnalyze("EXPLAIN ANALYZE CREATE TABLE " + tableName + " AS SELECT name FROM nation");
         assertQuery("SELECT * from " + tableName, "SELECT name FROM nation");
@@ -357,7 +354,6 @@ public void testCreateTable()
         assertQueryFails("CREATE TABLE " + tableName + " (a bad_type)", ".* Unknown type 'bad_type' for column 'a'");
         assertFalse(getQueryRunner().tableExists(getSession(), tableName));
 
-        // TODO (https://github.com/trinodb/trino/issues/5901) revert to longer name when Oracle version is updated
         tableName = "test_cr_not_exists_" + randomNameSuffix();
         assertUpdate("CREATE TABLE " + tableName + " (a bigint, b varchar(50), c double)");
         assertTrue(getQueryRunner().tableExists(getSession(), tableName));

diff --git a/plugin/trino-snowflake/src/test/java/io/trino/plugin/snowflake/SnowflakeQueryRunner.java b/plugin/trino-snowflake/src/test/java/io/trino/plugin/snowflake/SnowflakeQueryRunner.java
@@ -36,7 +36,6 @@ public final class SnowflakeQueryRunner
     private SnowflakeQueryRunner() {}
 
     public static DistributedQueryRunner createSnowflakeQueryRunner(
-            TestingSnowflakeServer server,
             Map<String, String> extraProperties,
             Map<String, String> connectorProperties,
             Iterable<TpchTable<?>> tables)
@@ -85,7 +84,6 @@ public static void main(String[] args)
             throws Exception
     {
         DistributedQueryRunner queryRunner = createSnowflakeQueryRunner(
-                new TestingSnowflakeServer(),
                 ImmutableMap.of("http-server.http.port", "8080"),
                 ImmutableMap.of(),
                 ImmutableList.of());

diff --git a/plugin/trino-snowflake/src/test/java/io/trino/plugin/snowflake/TestSnowflakeConfig.java b/plugin/trino-snowflake/src/test/java/io/trino/plugin/snowflake/TestSnowflakeConfig.java
@@ -32,8 +32,7 @@ public void testDefaults()
                 .setDatabase(null)
                 .setRole(null)
                 .setWarehouse(null)
-                .setHTTPProxy(null)
-                .setTimestampNoTimezoneAsUTC(null));
+                .setHTTPProxy(null));
     }
 
     @Test
@@ -53,8 +52,7 @@ public void testExplicitPropertyMappings()
                 .setDatabase("MYDATABASE")
                 .setRole("MYROLE")
                 .setWarehouse("MYWAREHOUSE")
-                .setHTTPProxy("MYPROXY")
-                .setTimestampNoTimezoneAsUTC(true);
+                .setHTTPProxy("MYPROXY");
 
         assertFullMapping(properties, expected);
     }

diff --git a/...n/trino-snowflake/src/test/java/io/trino/plugin/snowflake/TestSnowflakeConnectorTest.java b/...n/trino-snowflake/src/test/java/io/trino/plugin/snowflake/TestSnowflakeConnectorTest.java
@@ -26,8 +26,7 @@ public class TestSnowflakeConnectorTest
     protected QueryRunner createQueryRunner()
             throws Exception
     {
-        server = closeAfterClass(new TestingSnowflakeServer());
-        return createSnowflakeQueryRunner(server, ImmutableMap.of(), ImmutableMap.of(), REQUIRED_TPCH_TABLES);
+        return createSnowflakeQueryRunner(ImmutableMap.of(), ImmutableMap.of(), REQUIRED_TPCH_TABLES);
     }
 
     @Override

diff --git a/plugin/trino-snowflake/src/test/java/io/trino/plugin/snowflake/TestSnowflakeTypeMapping.java b/plugin/trino-snowflake/src/test/java/io/trino/plugin/snowflake/TestSnowflakeTypeMapping.java
@@ -65,7 +65,6 @@ public class TestSnowflakeTypeMapping
     @BeforeAll
     public void setUp()
     {
-        String zone = jvmZone.getId();
         checkState(jvmZone.getId().equals("America/Bahia_Banderas"), "Timezone not configured correctly. Add -Duser.timezone=America/Bahia_Banderas to your JVM arguments");
         checkIsGap(jvmZone, LocalDate.of(1970, 1, 1));
         checkIsGap(vilnius, LocalDate.of(1983, 4, 1));
@@ -76,9 +75,7 @@ public void setUp()
     protected QueryRunner createQueryRunner()
             throws Exception
     {
-        snowflakeServer = new TestingSnowflakeServer();
         return createSnowflakeQueryRunner(
-                snowflakeServer,
                 ImmutableMap.of(),
                 ImmutableMap.of(),
                 ImmutableList.of());
@@ -322,7 +319,6 @@ private void testTimestamp(ZoneId sessionZone)
                 .build();
 
         SqlDataTypeTest.create()
-                // after epoch (MariaDb's timestamp type doesn't support values <= epoch)
                 .addRoundTrip("timestamp(3)", "TIMESTAMP '2019-03-18 10:01:17.987'", createTimestampType(3), "TIMESTAMP '2019-03-18 10:01:17.987'")
                 // time doubled in JVM zone
                 .addRoundTrip("timestamp(3)", "TIMESTAMP '2018-10-28 01:33:17.456'", createTimestampType(3), "TIMESTAMP '2018-10-28 01:33:17.456'")