Add TestMemSqlDistributedQueries and implement gaps #5495

hashhar · 2020-10-09T15:02:06Z

findepi · 2020-10-09T21:44:07Z

i merged type mapping test, please rebase.

hashhar · 2020-10-10T10:51:55Z

Seems like MemSql tests were skipped. https://github.com/prestosql/presto/runs/1231980668?check_suite_focus=true#step:5:1
cc: @sopel39

Will rebase.

hashhar

I had some questions and have added them as comments.

presto-memsql/src/main/java/io/prestosql/plugin/memsql/MemSqlClient.java

presto-memsql/src/test/java/io/prestosql/plugin/memsql/TestMemSqlDistributedQueries.java

hashhar · 2020-10-12T09:18:38Z

presto-memsql/src/test/java/io/prestosql/plugin/memsql/TestMemSqlDistributedQueries.java

+    @Override
+    public void testLargeIn()
+    {
+        // TODO Caused by: java.sql.SQLException: Thread stack overrun:  Used: 1048713 of a 1048576 stack. Use 'memsqld --thread_stack=#' to specify a bigger stack if needed.


I opted to skip this test instead of modifying the test container since I don't think there's any point in making this test pass if it will actually fail in practice for most people.

presto-memsql/src/test/java/io/prestosql/plugin/memsql/TestMemSqlIntegrationSmokeTest.java

hashhar · 2020-10-12T09:29:13Z

presto-memsql/src/test/java/io/prestosql/plugin/memsql/TestMemSqlTypeMapping.java

@@ -95,7 +106,8 @@ private static DataTypeTest singlePrecisionFloatingPointTests(DataType<Float> fl
        // we are not testing Nan/-Infinity/+Infinity as those are not supported by MemSQL
        return DataTypeTest.create()
                .addRoundTrip(floatType, 3.14f)
-                // .addRoundTrip(floatType, 3.1415927f) // Overeagerly rounded by MemSQL to 3.14159 TODO
+                // TODO Overeagerly rounded by MemSQL to 3.14159


This seems to be due to MemSQL's FLOAT type having low precision rather than rounding errors.

hashhar · 2020-10-12T09:30:52Z

presto-memsql/src/test/java/io/prestosql/plugin/memsql/TestMemSqlTypeMapping.java

+    {
+        jsonTestCases(jsonDataType(value -> "JSON " + formatStringLiteral(value)))
+                .execute(getQueryRunner(), prestoCreateAsSelect("presto_test_json"));
+        // TODO MemSQL doesn't support CAST to JSON but accepts string literals as JSON values


CAST to JSON doesn't exist. There's a TO_JSON function but that parses strings into JSON values. ie. strings get converted into double quoted JSON strings. Passing strings that are valid JSON seem to get parsed into JSON during INSERT.

Having comment is good, but this shouldn't be a TODO, because that's nor our problem.

hashhar · 2020-10-12T09:45:40Z

@findepi Rebased on top of your PR. All tests in io.prestosql.plugin.memsql package are passing. Tested locally.

This PR will fix the following:

#5369
#5454
#5453

presto-memsql/src/main/java/io/prestosql/plugin/memsql/MemSqlClient.java

findepi

early feedback

presto-memsql/src/main/java/io/prestosql/plugin/memsql/MemSqlClient.java

hashhar · 2020-10-13T06:20:53Z

@findepi AC.

losipiuk · 2020-10-14T10:21:09Z

presto-memsql/src/main/java/io/prestosql/plugin/memsql/MemSqlClient.java

+        switch (typeHandle.getJdbcType()) {
+            case Types.VARCHAR:
+                VarcharType varcharType = (columnSize <= VarcharType.MAX_LENGTH) ? createVarcharType(columnSize) : createUnboundedVarcharType();
+                // Remote database can be case insensitive.


can we know if it is and react accordingly?

We can fire a query to test the easy cases (eg A = a) but it is possible for such checks to pass for ASCII characters but fail for unicode characters (eg. S = ß) depending on the collation used by the remote database.

I'm working on adding a smoke test to at-least verify that such-cases are caught in tests.

findepi · 2020-10-21T15:05:17Z

presto-memsql/src/main/java/io/prestosql/plugin/memsql/MemSqlClient.java

+    public PreparedStatement getPreparedStatement(Connection connection, String sql)
+            throws SQLException
+    {
+        return connection.prepareStatement(sql);


Override seems redundant?

findepi · 2020-10-21T15:06:32Z

presto-memsql/src/main/java/io/prestosql/plugin/memsql/MemSqlClient.java

+            else if (varcharType.getBoundedLength() <= 65535) {
+                dataType = "text";
+            }
+            else if (varcharType.getBoundedLength() <= 16777215) {
+                dataType = "mediumtext";


define constatns for these numbers, like you did for MEMSQL_VARCHAR_MAX_LENGTH

findepi · 2020-10-21T15:06:45Z

presto-memsql/src/main/java/io/prestosql/plugin/memsql/MemSqlClient.java

+            return WriteMapping.sliceMapping(dataType, varcharWriteFunction());
+        }
+        if (VARBINARY.equals(type)) {
+            return WriteMapping.sliceMapping("mediumblob", varbinaryWriteFunction());


medium or longblob?

findepi · 2020-10-21T15:06:59Z

presto-memsql/src/main/java/io/prestosql/plugin/memsql/MemSqlClient.java

+            return WriteMapping.longMapping("datetime", timestampWriteFunction(TIMESTAMP_MILLIS));
+        }
+
+        return super.toWriteMapping(session, type);


// TODO add explicit mappings

findepi · 2020-10-21T15:08:20Z

presto-memsql/src/main/java/io/prestosql/plugin/memsql/MemSqlClient.java

+    }
+
+    @Override
+    public void renameTable(JdbcIdentity identity, JdbcTableHandle handle, SchemaTableName newTableName)


move this above renameColumn (ideally, keep order as in superclass)

findepi · 2020-10-21T15:14:58Z

presto-memsql/src/test/java/io/prestosql/plugin/memsql/TestMemSqlTypeMapping.java

+        return dataType("date", DATE, toLiteral, identity());
+    }
+
+    private static DataType<String> memSqlJsonDataType(Function<String, String> toLiteral)


please do the rename jsonDataType -> memSqlJsonDataType
in the commit which intro'd the function

findepi · 2020-10-21T15:15:47Z

presto-memsql/src/test/java/io/prestosql/plugin/memsql/TestMemSqlDistributedQueries.java

-            return Optional.empty();
-        }
-        return super.filterColumnNameTestData(columnName);
+        return nullToEmpty(exception.getMessage()).matches(".*(Incorrect column name).*");


braces in the pattern are redundant.

findepi · 2020-10-21T15:16:32Z

presto-memsql/src/main/java/io/prestosql/plugin/memsql/MemSqlClient.java

+    public void createTable(ConnectorSession session, ConnectorTableMetadata tableMetadata)
+    {
+        // MemSQL doesn't accept `some;column` in CTAS statements - so we explicitly block it and throw a proper error message
+        // TODO figure out how to quote ; in column names


let's remove the TODO. I wouldn't spent time on it
same for copyTableSchema

findepi · 2020-10-21T15:17:26Z

presto-memsql/src/main/java/io/prestosql/plugin/memsql/MemSqlClient.java

+                .findAny();
+        if (illegalColumnName.isPresent()) {


you can get away without an assigment

.findAny() .ifPresent(illegalColumnName -> { throw ...

same in the other method

findepi · 2020-10-21T15:17:38Z

presto-memsql/src/main/java/io/prestosql/plugin/memsql/MemSqlClient.java

+    {
+        // MemSQL doesn't accept `some;column` in CTAS statements - so we explicitly block it and throw a proper error message
+        // TODO figure out how to quote ; in column names
+        Optional<String> illegalColumnName = columnNames.stream().filter(s -> s.contains(";")).findAny();


break the line before .filter and .findAny

Fixes #5263 Fixes #5369 Fixes #5452 Fixes #5453 Fixes #5454

hashhar · 2020-10-22T11:36:20Z

@findepi AC.

findepi · 2020-10-22T14:50:51Z

Thanks!

cla-bot bot added the cla-signed label Oct 9, 2020

hashhar requested a review from findepi October 9, 2020 15:29

findepi mentioned this pull request Oct 12, 2020

[Test] Add TestMemSqlDistributedQueries and implement gaps #5516

Closed

hashhar commented Oct 12, 2020

View reviewed changes

presto-memsql/src/main/java/io/prestosql/plugin/memsql/MemSqlClient.java Outdated Show resolved Hide resolved

findepi reviewed Oct 12, 2020

View reviewed changes

hashhar requested a review from sopel39 October 14, 2020 08:01

losipiuk reviewed Oct 14, 2020

View reviewed changes

findepi approved these changes Oct 21, 2020

View reviewed changes

hashhar added 5 commits October 22, 2020 17:05

Add TestMemSqlDistributedQueries and implement gaps

f53576d

Fixes #5263 Fixes #5369 Fixes #5452 Fixes #5453 Fixes #5454

Add MemSQL case-insensitive-name-matching tests

05d3040

Add MemSQL predicate pushdown tests

42cc099

Add MemSQL DATE tests

06dca4a

Disallow columns with ; in MemSQL

533a863

findepi merged commit 79e5ec8 into trinodb:master Oct 22, 2020

findepi mentioned this pull request Oct 22, 2020

Release notes for 345 #5503

Closed

7 tasks

findepi added this to the 345 milestone Oct 22, 2020

hashhar deleted the hashhar/memsql-distributed-tests branch October 23, 2020 04:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TestMemSqlDistributedQueries and implement gaps #5495

Add TestMemSqlDistributedQueries and implement gaps #5495

hashhar commented Oct 9, 2020

findepi commented Oct 9, 2020

hashhar commented Oct 10, 2020 •

edited

Loading

hashhar left a comment

hashhar Oct 12, 2020

hashhar Oct 12, 2020

hashhar Oct 12, 2020

findepi Oct 21, 2020

hashhar commented Oct 12, 2020

findepi left a comment

hashhar commented Oct 13, 2020

losipiuk Oct 14, 2020

hashhar Oct 14, 2020

findepi Oct 21, 2020

findepi Oct 21, 2020

findepi Oct 21, 2020

findepi Oct 21, 2020

findepi Oct 21, 2020

findepi Oct 21, 2020

findepi Oct 21, 2020

findepi Oct 21, 2020

findepi Oct 21, 2020

findepi Oct 21, 2020

hashhar commented Oct 22, 2020

findepi commented Oct 22, 2020

Add TestMemSqlDistributedQueries and implement gaps #5495

Add TestMemSqlDistributedQueries and implement gaps #5495

Conversation

hashhar commented Oct 9, 2020

findepi commented Oct 9, 2020

hashhar commented Oct 10, 2020 • edited Loading

hashhar 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

hashhar commented Oct 12, 2020

findepi left a comment

Choose a reason for hiding this comment

hashhar commented Oct 13, 2020

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

hashhar commented Oct 22, 2020

findepi commented Oct 22, 2020

hashhar commented Oct 10, 2020 •

edited

Loading