[DO NOT MERGE] - Debug flaky test

plugin/trino-clickhouse/pom.xml

@@ -46,27 +46,12 @@
         <dependency>
             <groupId>com.clickhouse</groupId>
             <artifactId>clickhouse-jdbc</artifactId>
-            <version>0.3.2-patch1</version>
+            <version>0.3.2-patch3</version>
+            <classifier>all</classifier>
             <exclusions>
                 <exclusion>
-                    <groupId>org.slf4j</groupId>
-                    <artifactId>slf4j-api</artifactId>
-                </exclusion>
-                <exclusion>
-                    <groupId>commons-logging</groupId>
-                    <artifactId>commons-logging</artifactId>
-                </exclusion>
-                <exclusion>
-                    <groupId>org.projectlombok</groupId>
-                    <artifactId>lombok</artifactId>
-                </exclusion>
-                <exclusion>
-                    <groupId>com.clickhouse</groupId>
-                    <artifactId>clickhouse-http-client</artifactId>
-                </exclusion>
-                <exclusion>
-                    <groupId>org.robolectric</groupId>
-                    <artifactId>android-all</artifactId>
+                    <groupId>*</groupId>
+                    <artifactId>*</artifactId>
                 </exclusion>
             </exclusions>
         </dependency>
@@ -202,4 +187,21 @@
             <scope>test</scope>
         </dependency>
     </dependencies>
+
+    <build>
+        <plugins>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-surefire-plugin</artifactId>
+                <dependencies>
+                    <!-- Specify TestNG explicitly for avoiding 'No tests were executed!' due to JUnit -->
+                    <dependency>
+                        <groupId>org.apache.maven.surefire</groupId>
+                        <artifactId>surefire-testng</artifactId>
+                        <version>${dep.plugin.surefire.version}</version>
+                    </dependency>
+                </dependencies>
+            </plugin>
+        </plugins>
+    </build>
 </project>

plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java

@@ -13,9 +13,12 @@
  */
 package io.trino.plugin.clickhouse;
 
+import com.clickhouse.client.ClickHouseColumn;
+import com.clickhouse.client.ClickHouseDataType;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.net.InetAddresses;
+import io.airlift.log.Logger;
 import io.airlift.slice.Slice;
 import io.trino.plugin.base.expression.AggregateFunctionRewriter;
 import io.trino.plugin.base.expression.AggregateFunctionRule;
@@ -61,10 +64,10 @@
 import java.net.InetAddress;
 import java.net.UnknownHostException;
 import java.sql.Connection;
-import java.sql.DatabaseMetaData;
 import java.sql.PreparedStatement;
 import java.sql.ResultSet;
 import java.sql.SQLException;
+import java.sql.Statement;
 import java.sql.Types;
 import java.time.LocalDate;
 import java.time.LocalDateTime;
@@ -142,6 +145,8 @@
 public class ClickHouseClient
         extends BaseJdbcClient
 {
+    private static final Logger log = Logger.get(ClickHouseClient.class);
+
     static final int CLICKHOUSE_MAX_DECIMAL_PRECISION = 76;
     private static final long MIN_SUPPORTED_DATE_EPOCH = LocalDate.parse("1970-01-01").toEpochDay();
     private static final long MAX_SUPPORTED_DATE_EPOCH = LocalDate.parse("2106-02-07").toEpochDay(); // The max date is '2148-12-31' in new ClickHouse version
@@ -342,16 +347,9 @@ public void setColumnComment(ConnectorSession session, JdbcTableHandle handle, J
     }
 
     @Override
-    public ResultSet getTables(Connection connection, Optional<String> schemaName, Optional<String> tableName)
-            throws SQLException
+    protected Optional<List<String>> getTableTypes()
     {
-        // ClickHouse maps their "database" to SQL catalogs and does not have schemas
-        DatabaseMetaData metadata = connection.getMetaData();
-        return metadata.getTables(
-                null,
-                schemaName.orElse(null),
-                escapeNamePattern(tableName, metadata.getSearchStringEscape()).orElse(null),
-                new String[] {"TABLE", "VIEW"});
+        return Optional.empty();
     }
 
     @Override
@@ -402,22 +400,24 @@ public Optional<ColumnMapping> toColumnMapping(ConnectorSession session, Connect
             return mapping;
         }
 
-        switch (jdbcTypeName.replaceAll("\\(.*\\)$", "")) {
-            case "IPv4":
+        ClickHouseColumn column = ClickHouseColumn.of("", jdbcTypeName);
+        ClickHouseDataType columnDataType = column.getDataType();
+        switch (columnDataType) {
+            case IPv4:
                 return Optional.of(ipAddressColumnMapping("IPv4StringToNum(?)"));
-            case "IPv6":
+            case IPv6:
                 return Optional.of(ipAddressColumnMapping("IPv6StringToNum(?)"));
-            case "Enum8":
-            case "Enum16":
+            case Enum8:
+            case Enum16:
                 return Optional.of(ColumnMapping.sliceMapping(
                         createUnboundedVarcharType(),
                         varcharReadFunction(createUnboundedVarcharType()),
                         varcharWriteFunction(),
                         // TODO (https://github.com/trinodb/trino/issues/7100) Currently pushdown would not work and may require a custom bind expression
                         DISABLE_PUSHDOWN));
 
-            case "FixedString": // FixedString(n)
-            case "String":
+            case FixedString: // FixedString(n)
+            case String:
                 if (isMapStringAsVarchar(session)) {
                     return Optional.of(ColumnMapping.sliceMapping(
                             createUnboundedVarcharType(),
@@ -427,7 +427,7 @@ public Optional<ColumnMapping> toColumnMapping(ConnectorSession session, Connect
                 }
                 // TODO (https://github.com/trinodb/trino/issues/7100) test & enable predicate pushdown
                 return Optional.of(varbinaryColumnMapping());
-            case "UUID":
+            case UUID:
                 return Optional.of(uuidColumnMapping());
         }
 
@@ -478,7 +478,7 @@ public Optional<ColumnMapping> toColumnMapping(ConnectorSession session, Connect
                 return Optional.of(dateColumnMappingUsingLocalDate());
 
             case Types.TIMESTAMP:
-                if (jdbcTypeName.equals("DateTime")) {
+                if (columnDataType == ClickHouseDataType.DateTime) {
                     verify(typeHandle.getRequiredDecimalDigits() == 0, "Expected 0 as timestamp precision, but got %s", typeHandle.getRequiredDecimalDigits());
                     return Optional.of(ColumnMapping.longMapping(
                             TIMESTAMP_SECONDS,
@@ -546,6 +546,27 @@ public WriteMapping toWriteMapping(ConnectorSession session, Type type)
         throw new TrinoException(NOT_SUPPORTED, "Unsupported column type: " + type);
     }
 
+    @Override
+    protected void execute(Connection connection, String query)
+    {
+        debug(connection);
+        super.execute(connection, query);
+        debug(connection);
+    }
+
+    private void debug(Connection connection)
+    {
+        try (Statement statement = connection.createStatement()) {
+            ResultSet resultSet = statement.executeQuery("SELECT version()");
+            while (resultSet.next()) {
+                log.info("Server version: %s", resultSet.getString(1));
+            }
+        }
+        catch (Exception e) {
+            throw new RuntimeException("Failed to execute statement", e);
+        }
+    }
+
     /**
      * format property to match ClickHouse create table statement
      *

plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClientModule.java

@@ -15,9 +15,7 @@
 
 import com.google.inject.Binder;
 import com.google.inject.Module;
-import com.google.inject.Provides;
 import com.google.inject.Scopes;
-import com.google.inject.Singleton;
 import io.airlift.configuration.ConfigBinder;
 import io.trino.plugin.jdbc.BaseJdbcConfig;
 import io.trino.plugin.jdbc.ConnectionFactory;
@@ -26,14 +24,23 @@
 import io.trino.plugin.jdbc.ForBaseJdbc;
 import io.trino.plugin.jdbc.JdbcClient;
 import io.trino.plugin.jdbc.credential.CredentialProvider;
-import ru.yandex.clickhouse.ClickHouseDriver;
+
+import javax.inject.Inject;
+import javax.inject.Provider;
+
+import java.sql.Driver;
 
 import static io.trino.plugin.jdbc.JdbcModule.bindSessionPropertiesProvider;
 import static io.trino.plugin.jdbc.JdbcModule.bindTablePropertiesProvider;
+import static java.lang.String.format;
 
 public class ClickHouseClientModule
         implements Module
 {
+    private static final String CLICKHOUSE_LATEST_DRIVER_CLASS_NAME = "com.clickhouse.jdbc.ClickHouseDriver";
+    // TODO: This Driver will not be available when clickhouse-jdbc is upgraded to 0.4.0 or above
+    private static final String CLICKHOUSE_DEPRECATED_DRIVER_CLASS_NAME = "ru.yandex.clickhouse.ClickHouseDriver";
+
     @Override
     public void configure(Binder binder)
     {
@@ -42,13 +49,53 @@ public void configure(Binder binder)
         binder.bind(JdbcClient.class).annotatedWith(ForBaseJdbc.class).to(ClickHouseClient.class).in(Scopes.SINGLETON);
         bindTablePropertiesProvider(binder, ClickHouseTableProperties.class);
         binder.install(new DecimalModule());
+        binder.bind(ConnectionFactory.class).annotatedWith(ForBaseJdbc.class).toProvider(ConnectionFactoryProvider.class).in(Scopes.SINGLETON);
     }
 
-    @Provides
-    @Singleton
-    @ForBaseJdbc
-    public static ConnectionFactory createConnectionFactory(BaseJdbcConfig config, CredentialProvider credentialProvider)
+    private static class ConnectionFactoryProvider
+            implements Provider<ConnectionFactory>
     {
-        return new ClickHouseConnectionFactory(new DriverConnectionFactory(new ClickHouseDriver(), config, credentialProvider));
+        private final ConnectionFactory connectionFactory;
+
+        @Inject
+        public ConnectionFactoryProvider(ClickHouseConfig clickHouseConfig, BaseJdbcConfig baseJdbcConfig, CredentialProvider credentialProvider)
+        {
+            connectionFactory = new ClickHouseConnectionFactory(
+                    new DriverConnectionFactory(createDriver(clickHouseConfig.isUseDeprecatedDriver()), baseJdbcConfig, credentialProvider));
+        }
+
+        @Override
+        public ConnectionFactory get()
+        {
+            return connectionFactory;
+        }
+
+        Driver createDriver(boolean useDeprecatedDriver)
+        {
+            String driverClass = getDriverClassName(useDeprecatedDriver);
+            try {
+                return (Driver) Class.forName(driverClass, true, getClassLoader()).getDeclaredConstructor().newInstance();
+            }
+            catch (ReflectiveOperationException e) {
+                throw new RuntimeException(format("Error creating an instance of %s", driverClass), e);
+            }
+        }
+
+        String getDriverClassName(boolean useDeprecatedDriver)
+        {
+            if (useDeprecatedDriver) {
+                return CLICKHOUSE_DEPRECATED_DRIVER_CLASS_NAME;
+            }
+            return CLICKHOUSE_LATEST_DRIVER_CLASS_NAME;
+        }
+
+        ClassLoader getClassLoader()
+        {
+            ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
+            if (classLoader == null) {
+                classLoader = ClickHouseConfig.class.getClassLoader();
+            }
+            return classLoader;
+        }
     }
 }

plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseConfig.java

@@ -21,6 +21,9 @@ public class ClickHouseConfig
     // TODO (https://github.com/trinodb/trino/issues/7102) reconsider default behavior
     private boolean mapStringAsVarchar;
 
+    // TODO: This config needs to be deprecated when we upgrade clickhouse-jdbc to version 0.4.0 or above
+    private boolean useDeprecatedDriver = true;
+
     public boolean isMapStringAsVarchar()
     {
         return mapStringAsVarchar;
@@ -33,4 +36,17 @@ public ClickHouseConfig setMapStringAsVarchar(boolean mapStringAsVarchar)
         this.mapStringAsVarchar = mapStringAsVarchar;
         return this;
     }
+
+    public boolean isUseDeprecatedDriver()
+    {
+        return useDeprecatedDriver;
+    }
+
+    @Config("clickhouse.use-deprecated-driver")
+    @ConfigDescription("Whether to use a deprecated driver")
+    public ClickHouseConfig setUseDeprecatedDriver(boolean useDeprecatedDriver)
+    {
+        this.useDeprecatedDriver = useDeprecatedDriver;
+        return this;
+    }
 }

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseConnectorSmokeTest.java

@@ -15,7 +15,6 @@
 
 import io.trino.plugin.jdbc.BaseJdbcConnectorSmokeTest;
 import io.trino.testing.TestingConnectorBehavior;
-import org.testng.annotations.Test;
 
 public abstract class BaseClickHouseConnectorSmokeTest
         extends BaseJdbcConnectorSmokeTest
@@ -31,8 +30,6 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
         }
     }
 
-    // TODO (https://github.com/trinodb/trino/issues/10653) Disable until fixing the flaky test issue
-    @Test(enabled = false)
     @Override
     public void testRenameSchema()
     {

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/ClickHouseQueryRunner.java

@@ -88,10 +88,11 @@ public static void main(String[] args)
     {
         Logging.initialize();
 
+        TestingClickHouseServer clickHouseServer = new TestingClickHouseServer();
         DistributedQueryRunner queryRunner = createClickHouseQueryRunner(
-                new TestingClickHouseServer(),
+                clickHouseServer,
                 ImmutableMap.of("http-server.http.port", "8080"),
-                ImmutableMap.of(),
+                ImmutableMap.of("clickhouse.use-deprecated-driver", String.valueOf(!clickHouseServer.isLatestDriverMinimumSupportedVersion())),
                 TpchTable.getTables());
 
         Logger log = Logger.get(ClickHouseQueryRunner.class);

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestAltinityConnectorSmokeTest.java

@@ -14,7 +14,9 @@
 package io.trino.plugin.clickhouse;
 
 import com.google.common.collect.ImmutableMap;
+import io.airlift.log.Logger;
 import io.trino.testing.QueryRunner;
+import org.testng.annotations.Test;
 
 import static io.trino.plugin.clickhouse.ClickHouseQueryRunner.createClickHouseQueryRunner;
 import static io.trino.plugin.clickhouse.TestingClickHouseServer.ALTINITY_DEFAULT_IMAGE;
@@ -23,19 +25,26 @@
 public class TestAltinityConnectorSmokeTest
         extends BaseClickHouseConnectorSmokeTest
 {
+    private static final Logger log = Logger.get(TestAltinityConnectorSmokeTest.class);
+
+    private TestingClickHouseServer clickHouseServer;
+
     @Override
     protected QueryRunner createQueryRunner()
             throws Exception
     {
+        clickHouseServer = closeAfterClass(new TestingClickHouseServer(ALTINITY_DEFAULT_IMAGE));
         return createClickHouseQueryRunner(
-                closeAfterClass(new TestingClickHouseServer(ALTINITY_DEFAULT_IMAGE)),
+                clickHouseServer,
                 ImmutableMap.of(),
                 ImmutableMap.<String, String>builder()
                         .put("clickhouse.map-string-as-varchar", "true") // To handle string types in TPCH tables as varchar instead of varbinary
+                        .put("clickhouse.use-deprecated-driver", String.valueOf(!clickHouseServer.isLatestDriverMinimumSupportedVersion()))
                         .buildOrThrow(),
                 REQUIRED_TPCH_TABLES);
     }
 
+    @Test
     @Override
     public void testRenameSchema()
     {

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConfig.java

@@ -42,18 +42,21 @@ public class TestClickHouseConfig
     public void testDefaults()
     {
         assertRecordedDefaults(recordDefaults(ClickHouseConfig.class)
-                .setMapStringAsVarchar(false));
+                .setMapStringAsVarchar(false)
+                .setUseDeprecatedDriver(true));
     }
 
     @Test
     public void testExplicitPropertyMappings()
     {
         Map<String, String> properties = new ImmutableMap.Builder<String, String>()
                 .put("clickhouse.map-string-as-varchar", "true")
+                .put("clickhouse.use-deprecated-driver", "false")
                 .buildOrThrow();
 
         ClickHouseConfig expected = new ClickHouseConfig()
-                .setMapStringAsVarchar(true);
+                .setMapStringAsVarchar(true)
+                .setUseDeprecatedDriver(false);
 
         assertFullMapping(properties, expected);
     }