Update memsql plugin JDBC driver dependency #11191

leveyja · 2022-02-25T09:42:54Z

Description

Update plugin dependency from MariaDB driver to official SingleStore (memsql) driver.

Related issues, pull requests, and links

Fixes Switch SingleStore (memsql) JDBC driver from MariaDB to official driver #10669

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

cla-bot · 2022-02-25T09:42:56Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

ebyhr · 2022-02-25T10:01:29Z

We need to update TestingMemSqlServer as well.

By the way, when I tried the official driver 1.0.1 last month, some of the ~~TestMemSqlConnectorTest~~ TestMemSqlTypeMapping failed. Can you run the test in your laptop because the MemSQL CI from forked repository doesn't run.

findepi · 2022-02-25T15:32:36Z

plugin/trino-memsql/pom.xml

+            <groupId>com.singlestore</groupId>
+            <artifactId>singlestore-jdbc-client</artifactId>
+            <version>1.0.1</version>


Is LGPL-2.1 a no-go, @martint ?

( https://search.maven.org/artifact/com.singlestore/singlestore-jdbc-client/1.0.1/jar )

I was going to ask if a new dependency needed to be vetted or documented somewhere (in our list of open source licenses?).
Btw, the MariaDB driver this is replacing is also LGPL-2.1:
( https://search.maven.org/artifact/org.mariadb.jdbc/mariadb-java-client/2.7.2/jar )
The SingleSource driver is forked from it.

Btw, the MariaDB driver this is replacing is also LGPL-2.1:

thanks for pointing this out.

I recall some offline discussion with the conclusion that LGPL dependency is OK, but still would prefer @martint to confirm. Will keep 'request for changes' for now here.

Thanks for raising it - I have this as a draft as I'm completely new to the process (and the codebase), so all feedback and issues are great for me to be aware of 👍

LGPL is ok. GPL is the one that we can't use, unless it has specific exclusions like JDK's Classpath Exception.

cla-bot · 2022-02-25T15:37:57Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

leveyja · 2022-02-25T16:02:03Z

We need to update TestingMemSqlServer as well.

By the way, when I tried the official driver 1.0.1 last month, some of the TestMemSqlConnectorTest failed. Can you run the test in your laptop because the MemSQL CI from forked repository doesn't run.

@ebyhr - Once I got the memsql.license I ran the tests locally and they passed 🟢

leveyja · 2022-02-25T16:03:38Z

~~Also - I submitted the [email protected] email 3 days ago - I'll resend it if there was been a problem~~
Update: This is all good (it's not an automated process)

ebyhr · 2022-02-25T21:51:16Z

@leveyja Sorry, I pasted the wrong test name. Could you confirm TestMemSqlTypeMapping?

cla-bot · 2022-02-28T11:48:38Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

jhlodin · 2022-02-28T18:16:48Z

Will users who currently specify the mariadb connection URL need to update?
Need to update the documentation here and possibly include a release note to inform users about the change - https://trino.io/docs/current/connector/memsql.html#configuration

hashhar · 2022-02-28T19:26:49Z

plugin/trino-memsql/src/main/java/io/trino/plugin/memsql/MemSqlClient.java

@@ -623,16 +623,16 @@ protected boolean isSupportedJoinCondition(JdbcJoinCondition joinCondition)
        }

        String typeName = typeHandle.getJdbcTypeName().get();
-        if (typeName.equalsIgnoreCase("tinyint unsigned")) {
+        if (typeName.equalsIgnoreCase("tinyint(3) unsigned")) {


Is it instead possible to create the proper JdbcTypeHandle for such columns in JdbcClient#getColumns? See for example how Postgres overrides that to fill in information about array types.

This seems prone to breakage and I'm not sure if this depends on the exact server and driver versions being used.

If this isn't possible to do then I suggest we drop this migration for now.

I'm of the opinion that if switching the driver isn't bringing any benefits at the moment then we should stick with the mariadb driver for now.
I looked at the current diff and don't see any improvements to consider the switch.

@hashhar - I had a questions with @wendigo about whether this list is exhaustive or not (I look in their source and I see tinyint(1)->tinyint(4), so I expect it is not). I agree it's fragile and could fail unexpectedly unless we had coverage of all possible types.
I'll look at the JdbcTypeHandle/Postgress client.getColumns - thanks for the suggestion!
(Whether we postpone the change or not, at least this is a useful learning experience for me! 👍 )

Considering the issues with the change to the connection string (thanks @jhlodin for raising that), it looks like a bigger change than originally envisioned, and for very little benefit (as you say, their current driver doesn't add much, and complicates the column mapping - future versions may change further, etc).

Maybe we can revisit this PR in the future once their driver matures a bit. WDYT @wendigo ?

Update / clarification: I'm going to attempt to update the type mapping as suggested, along with making jdbc:mariadb urls work transparently (that's already done w/tests, etc).
I may close this PR + re-open a new one though - so shout if I should do that! 👍

leveyja · 2022-03-01T07:19:03Z

Will users who currently specify the mariadb connection URL need to update? Need to update the documentation here and possibly include a release note to inform users about the change - https://trino.io/docs/current/connector/memsql.html#configuration

@jhlodin - The docs are generated from the repo, and the source of that doc was updated in the PR: see https://github.com/trinodb/trino/blob/d05536e90fc585cd5f61a1cf52ce28af363cd65a/docs/src/main/sphinx/connector/memsql.rst
(Sphinx generates the doc html from the markup in these rst files).

Re: Will users have to update the connection URL - I just did a quick test, and yes - they would need to update the URL if we go ahead and change our underlying driver to the singlestore driver - (unless we have a layer where we could 's/mariadb/singlestore/g' to the connection string within the MemSQL plugin? But that seems brittle).

In a case like this, where changing our driver would break consumers, do we have a preferred way of handling it? e.g., introduce a new setting to "update.mariadb.connection.to.singlestore=true" by default (and documenting that users should update their connection strings?).

Fixes trinodb#10669

cla-bot · 2022-03-02T16:51:48Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

leveyja · 2022-03-02T17:00:23Z

plugin/trino-memsql/src/main/java/io/trino/plugin/memsql/MemSqlClient.java

@@ -378,6 +382,46 @@ private static RemoteTableName getRemoteTable(ResultSet resultSet)
        return Optional.empty();
    }

+    public static ColumnMapping memsqlTimeColumnMapping(TimeType timeType) {


Please ignore this WIP / exploratory attempt to fix singlestore wrapping LocalTime types.
I may close this PR + open a new one when it's ready - there's a lot of work still to do to clean this up and test it.
CC: @wendigo @hashhar

leveyja · 2022-03-02T17:08:33Z

plugin/trino-memsql/src/main/java/io/trino/plugin/memsql/MemSqlClientModule.java

@@ -55,8 +57,14 @@ public static ConnectionFactory createConnectionFactory(BaseJdbcConfig config, C

        return new DriverConnectionFactory(
                new Driver(),
-                config.getConnectionUrl(),
+                ensureUrlBackwardCompatibility(config.getConnectionUrl()),


I noticed our config is mutable: so while config.getConnectionUrl() would still return "jdbc:mariadb", it would be possible to call config.setConnectionUrl(...), but this seems like it would cause impossible to diagnose issues (e.g., depending on when the Module is created, other components may have already "taken a copy" of the original url).

Should we create an ImmutableConfig wrapper around the config (override all .set methods w/throws UnsupportedOperationException("config is immutable")) to make sure it isn't changing after a certain point in the startup lifecycle?

Checkstyle fixes

leveyja · 2022-03-03T11:09:39Z

Closing in favour of new non-draft PR - #11301

findepi requested changes Feb 25, 2022

View reviewed changes

leveyja force-pushed the leveyja/10669-switch-mariadb-driver-to-singlestore branch from 309c04c to 0ad11fc Compare February 25, 2022 15:37

github-actions bot added the docs label Feb 25, 2022

findepi requested a review from hashhar February 28, 2022 16:14

hashhar reviewed Feb 28, 2022

View reviewed changes

leveyja added 2 commits March 2, 2022 17:51

Update memsql plugin JDBC driver dependency

8f14ae1

Fixes trinodb#10669

Ensure jdbc:mariadb connections still work

c6ba520

leveyja force-pushed the leveyja/10669-switch-mariadb-driver-to-singlestore branch from d05536e to c6ba520 Compare March 2, 2022 16:51

leveyja commented Mar 2, 2022

View reviewed changes

Attempt to fix unsigned type-mapping

853a1ce

cla-bot bot added the cla-signed label Mar 3, 2022

fixup! Attempt to fix unsigned type-mapping

faab8bc

Checkstyle fixes

leveyja closed this Mar 3, 2022

leveyja deleted the leveyja/10669-switch-mariadb-driver-to-singlestore branch March 3, 2022 11:10

leveyja mentioned this pull request Mar 3, 2022

Update memsql plugin JDBC driver dependency #11301

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update memsql plugin JDBC driver dependency #11191

Update memsql plugin JDBC driver dependency #11191

leveyja commented Feb 25, 2022

cla-bot bot commented Feb 25, 2022

ebyhr commented Feb 25, 2022 •

edited

Loading

findepi Feb 25, 2022 •

edited

Loading

leveyja Feb 25, 2022

findepi Feb 25, 2022

leveyja Feb 25, 2022

martint Mar 11, 2022 •

edited

Loading

cla-bot bot commented Feb 25, 2022

leveyja commented Feb 25, 2022 •

edited

Loading

leveyja commented Feb 25, 2022 •

edited

Loading

ebyhr commented Feb 25, 2022 •

edited

Loading

cla-bot bot commented Feb 28, 2022

jhlodin commented Feb 28, 2022 •

edited

Loading

hashhar Feb 28, 2022

hashhar Feb 28, 2022

leveyja Mar 1, 2022

leveyja Mar 1, 2022

hashhar Mar 1, 2022

leveyja Mar 2, 2022

leveyja commented Mar 1, 2022 •

edited

Loading

cla-bot bot commented Mar 2, 2022

leveyja Mar 2, 2022

leveyja Mar 2, 2022

leveyja commented Mar 3, 2022 •

edited

Loading

Update memsql plugin JDBC driver dependency #11191

Update memsql plugin JDBC driver dependency #11191

Conversation

leveyja commented Feb 25, 2022

Description

Related issues, pull requests, and links

Documentation

Release notes

cla-bot bot commented Feb 25, 2022

ebyhr commented Feb 25, 2022 • edited Loading

findepi Feb 25, 2022 • edited Loading

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

martint Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

cla-bot bot commented Feb 25, 2022

leveyja commented Feb 25, 2022 • edited Loading

leveyja commented Feb 25, 2022 • edited Loading

ebyhr commented Feb 25, 2022 • edited Loading

cla-bot bot commented Feb 28, 2022

jhlodin commented Feb 28, 2022 • edited Loading

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

leveyja commented Mar 1, 2022 • edited Loading

cla-bot bot commented Mar 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leveyja commented Mar 3, 2022 • edited Loading

ebyhr commented Feb 25, 2022 •

edited

Loading

findepi Feb 25, 2022 •

edited

Loading

martint Mar 11, 2022 •

edited

Loading

leveyja commented Feb 25, 2022 •

edited

Loading

leveyja commented Feb 25, 2022 •

edited

Loading

ebyhr commented Feb 25, 2022 •

edited

Loading

jhlodin commented Feb 28, 2022 •

edited

Loading

leveyja commented Mar 1, 2022 •

edited

Loading

leveyja commented Mar 3, 2022 •

edited

Loading