Add unit tests for SqlServer #503

kokosing · 2019-03-19T08:52:57Z

Add unit tests for SqlServer

kokosing · 2019-03-19T08:58:40Z

This is first attempt to use docker image in unit tests. Things to improve:

use dynamic RDBMS port on host
then we can run a sql server docker container per each test class.

findepi · 2019-03-19T14:11:58Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/QueryBuilder.java

                domain = pushDownDomain(client, session, column, domain);
                builder.add(toPredicate(column.getColumnName(), domain, column, accumulator));
            }
        }
        return builder.build();
    }

+    protected boolean isAcceptedDomain(Domain domain)


Revert, this is a responsibility of pushDownDomain.

findepi · 2019-03-19T14:13:36Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/QueryBuilder.java

@@ -266,8 +266,8 @@ private String toPredicate(String columnName, String operator, Object value, Jdb

    private String quote(String name)
    {
-        name = name.replace(quote, quote + quote);
-        return quote + name + quote;
+        name = name.replace(identifierQoute, identifierQoute + identifierQoute);


findepi · 2019-03-19T14:14:42Z

presto-sqlserver/src/main/java/io/prestosql/plugin/sqlserver/SqlServerQueryBuilder.java

+public class SqlServerQueryBuilder
+        extends QueryBuilder
+{
+    // SqlServer supports 2100 parameters in preparared statement, 100 is left for other parameters


so we would support queries with 1 large IN but not queries with 2 INs?

findepi · 2019-03-19T14:18:55Z

presto-sqlserver/src/main/java/io/prestosql/plugin/sqlserver/SqlServerClient.java

+    public PreparedStatement buildSql(ConnectorSession session, Connection connection, JdbcSplit split, List<JdbcColumnHandle> columnHandles)
+            throws SQLException
+    {
+        return new SqlServerQueryBuilder().buildSql(


Now the identifierQuote selection logic is in 2 places:

SqlServerQueryBuilder::new

SqlServerClient::new

IMO it would be better to pass this.identifierQuote to the QueryBuilder constructor

True. Let's solve it separate PR.

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

findepi · 2019-03-19T14:33:36Z

presto-sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestingSqlServerServer.java

+            throws Exception
+    {
+        this.dockerContainer = new TemporaryDockerContainer(
+                "microsoft/mssql-server-linux:2017-CU6",


This is not the version we're using in product tests.

findepi · 2019-03-19T14:34:00Z

presto-sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestingSqlServerServer.java

+
+        RetryPolicy<Object> retryPolicy = new RetryPolicy<>()
+                .withMaxDuration(Duration.of(5, MINUTES))
+                .withMaxAttempts(Integer.MAX_VALUE) // limited by MaxDuration


What would be the default if you don't set this?

findepi · 2019-03-19T14:34:12Z

presto-sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestingSqlServerServer.java

+        RetryPolicy<Object> retryPolicy = new RetryPolicy<>()
+                .withMaxDuration(Duration.of(5, MINUTES))
+                .withMaxAttempts(Integer.MAX_VALUE) // limited by MaxDuration
+                .onRetry(event -> LOG.info("Waiting for sql-server..."))


"SQL Server" ?

presto-sqlserver/pom.xml

findepi · 2019-03-19T14:35:15Z

presto-sqlserver/pom.xml

+        </dependency>
+        <dependency>
+            <groupId>io.prestosql</groupId>
+            <artifactId>presto-tests</artifactId>


move this up, eg after presto-main

kokosing

@findepi Comments addressed.

kokosing · 2019-03-25T09:29:23Z

presto-sqlserver/src/main/java/io/prestosql/plugin/sqlserver/SqlServerClient.java

+    public PreparedStatement buildSql(ConnectorSession session, Connection connection, JdbcSplit split, List<JdbcColumnHandle> columnHandles)
+            throws SQLException
+    {
+        return new SqlServerQueryBuilder().buildSql(


True. Let's solve it separate PR.

kokosing · 2019-03-25T10:18:28Z

presto-sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestingSqlServerServer.java

+
+        RetryPolicy<Object> retryPolicy = new RetryPolicy<>()
+                .withMaxDuration(Duration.of(5, MINUTES))
+                .withMaxAttempts(Integer.MAX_VALUE) // limited by MaxDuration


kokosing · 2019-03-25T10:22:25Z

...sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestSqlServerIntegrationSmokeTest.java

+    }
+
+    @Test
+    public void testDropTable()


Let me fix this in separate PR. Add this AbstractTestIntegrationSmokeTest could blow up.

kokosing · 2019-03-25T10:22:58Z

...sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestSqlServerIntegrationSmokeTest.java

+    @Test
+    public void testInsert()
+    {
+        executeInSqlServer("CREATE TABLE test_insert (x bigint, y varchar(100))");


Why don't you CREATE TABLE in Presto?

Because test is focused on INSERT.

findepi

A few comments about TemporaryDockerContainer

This is really nice!

findepi · 2019-03-25T15:44:08Z

presto-sqlserver/src/main/java/io/prestosql/plugin/sqlserver/SqlServerClient.java


 public class SqlServerClient
        extends BaseJdbcClient
 {
    private static final Joiner DOT_JOINER = Joiner.on(".");

+    private static final Map<Type, WriteMapping> WRITE_MAPPINGS = ImmutableMap.<Type, WriteMapping>builder()


Does sql server have all the types used here?

I do not know. There is no logical change here. I don't want to fix mapping in SqlServer now.

I guess so. So let's not inline this from basejdbc.
Instead, you can call super and then add DISABLE_UNSUPPORTED_PUSHDOWN. This is a hack, but at least it does not pretend it is not a hack. You then can adorn the code with // TODO implement explicit type mappings.

That was first idea. However, after our offline discussion I understood that you prefer inlinining. Maybe, it was because I promised you that I will handle mapping end-to-end. I would like to, but I don't have time right now for this.

Apologies for mis-communication.

Ideally we should inline & retain only the necessary bits. And add TypeMapping test.

I am aware you might not have the cycles to reach this ideal, so I would really prefer no inlining at all.

findepi · 2019-03-25T15:44:35Z

presto-sqlserver/src/main/java/io/prestosql/plugin/sqlserver/SqlServerClient.java

+        int columnSize = type.getColumnSize();
+        switch (type.getJdbcType()) {
+            case Types.BIT:
+            case Types.BOOLEAN:


Does sql server have bit data type? And boolean?

Same for all the mappings.

findepi · 2019-03-25T15:45:38Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/StandardColumnMappings.java

+        return decimalColumnMapping(decimalType, UnaryOperator.identity());
+    }
+
+    public static ColumnMapping decimalColumnMapping(DecimalType decimalType, UnaryOperator<Domain> pushDownConverter)


nit: pushDownConverter → pushdownConverter

However, i think this is a wrong abstraction level.
Why do you need this?

I added this to avoid inlining more code into SqlServer.

Please see 45e0c2f#r268709657. Then you should be able to revert this change.

Is it possible to revert changes in StandardColumnMappings.java now?
they are in Fix large IN queries agains SqlServer cmt, but look no longer needed here.

presto-sqlserver/src/main/java/io/prestosql/plugin/sqlserver/SqlServerClient.java

findepi · 2019-03-25T16:16:45Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

+
+    private int extractPort(Map.Entry<String, List<PortBinding>> entry)
+    {
+        return Integer.parseInt(entry.getKey().replace("/tcp", "").replace("/udp", ""));


AFAIK, Docker for Mac does not handle UDP correctly, so let's not pretend this is supported.

What happens when eg 88/tcp and 88/udp are bound but mapped to a different host port?

I removed UDP support

findepi · 2019-03-25T16:21:04Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

+                .collect(toImmutableMap(
+                        entry -> extractPort(entry),
+                        entry -> entry.getValue().stream()
+                                .filter(portBinding -> portBinding.hostIp().equals(hostIp))


Can there by anything else?

I do not think so.

findepi · 2019-03-25T16:22:39Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

+                                .map(PortBinding::hostPort)
+                                .collect(toOptional())
+                                .map(Integer::parseInt)
+                                .orElseThrow(() -> new IllegalStateException("Could not find port mapping for: " + entry.getKey()))));


"Could not extract port mapping from " + entry

findepi · 2019-03-25T16:23:12Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

+    }
+
+    @Override
+    public void close()


move it as the last instance method

findepi · 2019-03-25T16:23:56Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

+        LOG.info("Auto-assigned host ports are %s", hostPorts);
+    }
+
+    public int getHostPort(int port)


Move it below calculateHostPorts

presto-sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestingSqlServerServer.java

findepi · 2019-03-25T16:28:02Z

presto-sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestingSqlServerServer.java

+    public static final String PASSWORD = "SQLServerPass1";
+
+    private static TemporaryDockerContainer dockerContainer = new TemporaryDockerContainer(
+            "microsoft/mssql-server-linux:2017-CU6",


outdated version

findepi · 2019-03-25T16:28:09Z

presto-sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestingSqlServerServer.java

+
+    private static TemporaryDockerContainer dockerContainer = new TemporaryDockerContainer(
+            "microsoft/mssql-server-linux:2017-CU6",
+            "sqlserver-master",


just sqlserver

findepi · 2019-03-25T16:31:21Z

presto-sqlserver/src/test/java/io/prestosql/plugin/sqlserver/SqlServerQueryRunner.java

+    {
+        TestingSqlServerServer.waitForSqlServer();
+
+        DistributedQueryRunner queryRunner = new DistributedQueryRunner(createSession(), 3);


this is deprecated (😞)

Maybe simply undeprecate this?

findepi · 2019-03-25T16:32:55Z

presto-sqlserver/src/test/java/io/prestosql/plugin/sqlserver/SqlServerQueryRunner.java

+        }
+    }
+
+    private static synchronized void provisionTables(Session session, QueryRunner queryRunner, Iterable<TpchTable<?>> tables)


Make sure this is no longer static synchronized when you move container init to be once-per-test-class.

findepi · 2019-03-25T16:33:29Z

...sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestSqlServerIntegrationSmokeTest.java

+    }
+
+    @Test
+    public void testDropTable()


Fine, so don't add them here.

findepi · 2019-03-25T16:34:09Z

...sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestSqlServerIntegrationSmokeTest.java

+
+    private AutoCloseable withTable(String tableName, String tableDefinition)
+    {
+        executeInSqlServer(format("CREATE TABLE %s%s", tableName, tableDefinition));


"CREATE TABLE %s %s" would be slightly more readable

findepi · 2019-03-26T15:42:15Z

pom.xml

@@ -110,6 +110,7 @@
        <module>presto-geospatial</module>
        <module>presto-geospatial-toolkit</module>
        <module>presto-benchmark</module>
+        <module>presto-testing-docker</module>


Do we need a new module for this? Can't we put it in presto-tests?

Yes, we do. Otherwise it fails due conflict of classes and native libs with cassandra. I tried to solve this, but shading native libraries is not an easy thing to do.

kokosing

Addressed comments as fixups

kokosing · 2019-03-27T10:02:04Z

presto-sqlserver/src/main/java/io/prestosql/plugin/sqlserver/SqlServerClient.java


 public class SqlServerClient
        extends BaseJdbcClient
 {
    private static final Joiner DOT_JOINER = Joiner.on(".");

+    private static final Map<Type, WriteMapping> WRITE_MAPPINGS = ImmutableMap.<Type, WriteMapping>builder()


I do not know. There is no logical change here. I don't want to fix mapping in SqlServer now.

kokosing · 2019-03-27T10:03:03Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/StandardColumnMappings.java

+        return decimalColumnMapping(decimalType, UnaryOperator.identity());
+    }
+
+    public static ColumnMapping decimalColumnMapping(DecimalType decimalType, UnaryOperator<Domain> pushDownConverter)


I added this to avoid inlining more code into SqlServer.

presto-sqlserver/src/main/java/io/prestosql/plugin/sqlserver/SqlServerClient.java

kokosing · 2019-03-27T10:10:50Z

pom.xml

@@ -110,6 +110,7 @@
        <module>presto-geospatial</module>
        <module>presto-geospatial-toolkit</module>
        <module>presto-benchmark</module>
+        <module>presto-testing-docker</module>


Yes, we do. Otherwise it fails due conflict of classes and native libs with cassandra. I tried to solve this, but shading native libraries is not an easy thing to do.

kokosing · 2019-03-27T10:12:09Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

+    public TemporaryDockerContainer(String image, String hostName, List<Integer> ports, Map<String, String> environment)
+    {
+        this.image = requireNonNull(image, "image is null");
+        this.hostName = requireNonNull(hostName, "hostName is null");


It is used internally, I can remove it from here for now.

kokosing · 2019-03-27T10:21:53Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

+                .withMaxDuration(Duration.of(5, MINUTES))
+                .withMaxAttempts(Integer.MAX_VALUE) // limited by MaxDuration
+                .onRetry(event -> LOG.info(format("Waiting for %s...", hostName)))
+                .withDelay(Duration.of(5, SECONDS));


Travis does not complain for such things. It would complain if I would raise it above 10 minutes. Anyway, I changed this to 10s.

kokosing · 2019-03-27T10:52:29Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

+
+        ports.forEach(port -> {
+            RetryPolicy<Object> retryPolicy = new RetryPolicy<>()
+                    .withMaxDuration(Duration.of(5, MINUTES))


The thing this the same value is used twice, does not mean it is the same timeout. For me waiting on container or a service in it is a bit different.

kokosing · 2019-03-27T10:55:05Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

+                .collect(toImmutableMap(
+                        entry -> extractPort(entry),
+                        entry -> entry.getValue().stream()
+                                .filter(portBinding -> portBinding.hostIp().equals(hostIp))


I do not think so.

kokosing · 2019-03-27T10:58:13Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

+
+    private int extractPort(Map.Entry<String, List<PortBinding>> entry)
+    {
+        return Integer.parseInt(entry.getKey().replace("/tcp", "").replace("/udp", ""));


I removed UDP support

kokosing · 2019-03-27T10:59:52Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

+        dockerClient = null;
+    }
+
+    protected boolean isDebug()


It used to be more complicated. It was checking environment variables.

findepi · 2019-03-27T12:47:06Z

presto-sqlserver/src/main/java/io/prestosql/plugin/sqlserver/SqlServerClient.java


 public class SqlServerClient
        extends BaseJdbcClient
 {
    private static final Joiner DOT_JOINER = Joiner.on(".");

+    private static final Map<Type, WriteMapping> WRITE_MAPPINGS = ImmutableMap.<Type, WriteMapping>builder()


I guess so. So let's not inline this from basejdbc.
Instead, you can call super and then add DISABLE_UNSUPPORTED_PUSHDOWN. This is a hack, but at least it does not pretend it is not a hack. You then can adorn the code with // TODO implement explicit type mappings.

findepi · 2019-03-27T12:47:45Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/StandardColumnMappings.java

+        return decimalColumnMapping(decimalType, UnaryOperator.identity());
+    }
+
+    public static ColumnMapping decimalColumnMapping(DecimalType decimalType, UnaryOperator<Domain> pushDownConverter)


Please see 45e0c2f#r268709657. Then you should be able to revert this change.

findepi · 2019-03-27T12:48:37Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

        implements Closeable
 {
+
+    private static final boolean DEBUG = false;


put after logger (logger should be the first)

I really don't know why logger is that important in every class. For me it should be defined at the bottom.

yeah, i know some folks want to put weird things at the bottom. Like private Ctor() {}.
However, it's not about importance. It's just a convention -- i am used to look at the top to see wether a logger is already defined. Or "utility class constructor".

findepi · 2019-03-27T12:49:42Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

    private Map<Integer, Integer> hostPorts;

-    public TemporaryDockerContainer(String image, String hostName, List<Integer> ports, Map<String, String> environment)
+    public TemporaryDockerContainer(String image, List<Integer> ports, Map<String, String> environment, CheckedConsumer<HostPortProvider> verifier)


verifier -> healthCheck ?

findepi · 2019-03-27T12:51:09Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

+        Failsafe.with(retryPolicy).run(() -> verifier.accept(this::getHostPort));
+    }
+
+    private boolean isContainerUp()


move it before waitForContainer since it's called earlier

same for createContainer

findepi · 2019-03-27T12:58:01Z

presto-testing-docker/src/main/java/io/prestosql/testing/HostPortProvider.java

+ */
+package io.prestosql.testing;
+
+public interface HostPortProvider


nit: you can define this as an inner class in TemporaryDockerContainer.

findepi · 2019-03-27T12:58:23Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

@@ -24,112 +24,119 @@
 import io.airlift.log.Logger;


change package name to io.prestosql.testing.docker -- presto-testing-docker module should have its own "namespace"

findepi · 2019-03-27T12:59:30Z

presto-sqlserver/src/test/java/io/prestosql/plugin/sqlserver/SqlServerQueryRunner.java

+    {
+        TestingSqlServerServer.waitForSqlServer();
+
+        DistributedQueryRunner queryRunner = new DistributedQueryRunner(createSession(), 3);


Maybe simply undeprecate this?

findepi · 2019-03-27T13:02:47Z

presto-sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestingSqlServer.java

    {
-        return format("jdbc:sqlserver://localhost:%s", dockerContainer.getHostPort(1433));
+        this.dockerContainer = new TemporaryDockerContainer(
+                "microsoft/mssql-server-linux:2017-CU6",


722ce95#r268733179

See https://github.com/prestosql/presto/blob/master/presto-product-tests/conf/docker/singlenode-sqlserver/docker-compose.yml#L6

findepi · 2019-03-27T13:05:51Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

+    public TemporaryDockerContainer(String image, String hostName, List<Integer> ports, Map<String, String> environment)
+    {
+        this.image = requireNonNull(image, "image is null");
+        this.hostName = requireNonNull(hostName, "hostName is null");


If it is needed internally, then i'm OK

(i am aware you removed this... so up to you)

kokosing

Comments addressed

kokosing · 2019-03-27T20:01:21Z

presto-sqlserver/src/main/java/io/prestosql/plugin/sqlserver/SqlServerClient.java


 public class SqlServerClient
        extends BaseJdbcClient
 {
    private static final Joiner DOT_JOINER = Joiner.on(".");

+    private static final Map<Type, WriteMapping> WRITE_MAPPINGS = ImmutableMap.<Type, WriteMapping>builder()


That was first idea. However, after our offline discussion I understood that you prefer inlinining. Maybe, it was because I promised you that I will handle mapping end-to-end. I would like to, but I don't have time right now for this.

kokosing · 2019-03-27T20:05:14Z

presto-testing-docker/src/main/java/io/prestosql/testing/TemporaryDockerContainer.java

        implements Closeable
 {
+
+    private static final boolean DEBUG = false;


I really don't know why logger is that important in every class. For me it should be defined at the bottom.

kokosing · 2019-03-27T20:35:17Z

...sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestSqlServerIntegrationSmokeTest.java

+    }
+
+    @Test
+    public void testDropTable()


Why? It is better to have test for single connector vs for none.

kokosing · 2019-03-27T20:38:31Z

...sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestSqlServerIntegrationSmokeTest.java

+    @Test
+    public void testInsert()
+    {
+        executeInSqlServer("CREATE TABLE test_insert (x bigint, y varchar(100))");


I will move it to AbstractTestIntegrationSmokeTest in separate PR.

findepi

Good job!

findepi · 2019-03-27T21:30:16Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/ColumnMapping.java

@@ -81,7 +81,11 @@ public static ColumnMapping blockMapping(Type prestoType, BlockReadFunction read
    private final WriteFunction writeFunction;
    private final UnaryOperator<Domain> pushdownConverter;

-    private ColumnMapping(Type type, ReadFunction readFunction, WriteFunction writeFunction, UnaryOperator<Domain> pushdownConverter)
+    /**
+     * Prefer factory methods instead over calling constructor directly.


@deprecated Prefer factory methods ...

findepi · 2019-03-27T21:30:45Z

presto-sqlserver/src/main/java/io/prestosql/plugin/sqlserver/SqlServerClient.java

@@ -37,6 +43,17 @@
 {
    private static final Joiner DOT_JOINER = Joiner.on(".");

+    // SqlServer supports 2100 parameters in prepared statement, let's create a space for for about 4 big IN predicates
+    static final int SQL_SERVER_MAX_LIST_EXPRESSIONS = 500;


private or @VisibleForTesting

findepi · 2019-03-27T21:30:52Z

presto-sqlserver/src/main/java/io/prestosql/plugin/sqlserver/SqlServerClient.java


 import javax.inject.Inject;

 import java.sql.Connection;
 import java.sql.SQLException;
+import java.util.Optional;
+import java.util.function.UnaryOperator;


Fix large IN queries agains SqlServer

typo in cmt msg: "against"

findepi · 2019-03-27T21:31:20Z

presto-sqlserver/src/main/java/io/prestosql/plugin/sqlserver/SqlServerClient.java

@@ -37,6 +43,17 @@
 {
    private static final Joiner DOT_JOINER = Joiner.on(".");

+    // SqlServer supports 2100 parameters in prepared statement, let's create a space for for about 4 big IN predicates


findepi · 2019-03-27T21:32:31Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/StandardColumnMappings.java

+        return decimalColumnMapping(decimalType, UnaryOperator.identity());
+    }
+
+    public static ColumnMapping decimalColumnMapping(DecimalType decimalType, UnaryOperator<Domain> pushDownConverter)


Is it possible to revert changes in StandardColumnMappings.java now?
they are in Fix large IN queries agains SqlServer cmt, but look no longer needed here.

findepi · 2019-03-27T21:33:33Z

presto-testing-docker/src/main/java/io/prestosql/testing/docker/DockerContainer.java

+import static java.time.temporal.ChronoUnit.SECONDS;
+import static java.util.Objects.requireNonNull;
+
+public final class DockerContainer


Thanks for the rename.

now the cmt msg Introduce TemporaryDockerContainer can be changed to Introduce DockerContainer for testing or sth like that.

findepi · 2019-03-27T21:34:23Z

presto-testing-docker/src/main/java/io/prestosql/testing/docker/DockerContainer.java

+            createContainer(ports);
+        }
+
+        checkState(isContainerUp(), "Container was not started properly");


If this fails, will the container be removed?

Yes. Tested manually.

findepi · 2019-03-27T21:34:43Z

presto-testing-docker/src/main/java/io/prestosql/testing/docker/DockerContainer.java

+
+        LOG.info("Auto-assigned host ports are %s", hostPorts);
+
+        waitForContainer(healthCheck);


If this fails, will the container be removed?

Yes. Thanks for asking. It appeared that when method throws in abortOn then failsafe continue to retry. I fixed that.

findepi · 2019-03-27T21:37:15Z

presto-sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestingSqlServer.java

+
+    private static void execute(HostPortProvider hostPortProvider, String sql)
+    {
+        try (Connection conn = DriverManager.getConnection(getJdbcUrl(hostPortProvider.getHostPort(SQL_SERVER_PORT)), USER, PASSWORD);


nit: extract getJdbcUrl(hostPortProvider.getHostPort(SQL_SERVER_PORT)) as a variable?

This way it is consistent with BaseJdbcClient

DockerContainer is an utility to make it easier to use external services managed provided with docker images for unit testing.

kokosing

Comments addressed Thanks for reviewing.

kokosing · 2019-03-28T05:04:49Z

presto-testing-docker/src/main/java/io/prestosql/testing/docker/DockerContainer.java

+            createContainer(ports);
+        }
+
+        checkState(isContainerUp(), "Container was not started properly");


Yes. Tested manually.

kokosing · 2019-03-28T05:27:27Z

presto-testing-docker/src/main/java/io/prestosql/testing/docker/DockerContainer.java

+
+        LOG.info("Auto-assigned host ports are %s", hostPorts);
+
+        waitForContainer(healthCheck);


Yes. Thanks for asking. It appeared that when method throws in abortOn then failsafe continue to retry. I fixed that.

cla-bot bot added the cla-signed label Mar 19, 2019

kokosing requested review from electrum and findepi March 19, 2019 08:56

findepi requested changes Mar 19, 2019

View reviewed changes

kokosing mentioned this pull request Mar 25, 2019

[WIP] adding druid plugin #532

Closed

kokosing force-pushed the origin/master/110_unit_tests_for_sqlserver branch from f4ab75e to 827328c Compare March 25, 2019 10:39

kokosing commented Mar 25, 2019

View reviewed changes

findepi reviewed Mar 25, 2019

View reviewed changes

findepi reviewed Mar 26, 2019

View reviewed changes

kokosing force-pushed the origin/master/110_unit_tests_for_sqlserver branch from 827328c to 263d4b3 Compare March 27, 2019 11:53

kokosing commented Mar 27, 2019

View reviewed changes

findepi reviewed Mar 27, 2019

View reviewed changes

kokosing force-pushed the origin/master/110_unit_tests_for_sqlserver branch from 263d4b3 to 1016708 Compare March 27, 2019 21:21

kokosing commented Mar 27, 2019

View reviewed changes

findepi approved these changes Mar 27, 2019

View reviewed changes

kokosing added 4 commits March 28, 2019 06:30

Rename quote to identifierQoute in QueryBuilder

b12b31b

This way it is consistent with BaseJdbcClient

Fix large IN queries against SqlServer

0fb0700

Introduce DockerContainer for testing

d09db94

DockerContainer is an utility to make it easier to use external services managed provided with docker images for unit testing.

Add unit tests for SqlServer

ca154eb

kokosing force-pushed the origin/master/110_unit_tests_for_sqlserver branch from 1016708 to ca154eb Compare March 28, 2019 05:34

kokosing commented Mar 28, 2019

View reviewed changes

kokosing merged commit d1bec03 into trinodb:master Mar 28, 2019

kokosing deleted the origin/master/110_unit_tests_for_sqlserver branch March 28, 2019 08:23


		LOG.info("Auto-assigned host ports are %s", hostPorts);

		waitForContainer(healthCheck);

Add unit tests for SqlServer #503

Add unit tests for SqlServer #503

Conversation

kokosing commented Mar 19, 2019

kokosing commented Mar 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment