Update memsql plugin JDBC driver dependency

[leveyja]

Fixes #10669

Description

Upgrade MemSQL Plugin dependency from MariaDB driver to SingleStore driver.

Is this change a fix, improvement, new feature, refactoring, or other?

Improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Connector / MemSQL Plugin

How would you describe this change to a non-technical end user or system administrator?

Updating Plugin driver dependency to use official SingleStore driver which is under active development and supersedes previous MariaDB driver

Related issues, pull requests, and links

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

Documentation

( ) No documentation is needed.
(x) 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.
(x) Release notes entries required with the following suggested text:

## SingleStore (MemSQL) connector
* The connector now uses the official [Single Store JDBC Driver](https://docs.singlestore.com/managed-service/en/developer-resources/connect-with-application-development-tools/connect-with-java-jdbc/the-singlestore-jdbc-driver.html). As a result, `connection-url` in catalog 
  configuration files needs to be updated from `jdbc:mariadb:...` to `jdbc:singlestore:...`. ({issue}`10669`)

[leveyja]

@findepi @martint
Piotr raised a question around the new driver's license (LGPL-2.1) -

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

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

It turns out the current mariadb driver is also LGPL-2.1 (so no change in license), but I wanted to preserve the question on the new PR - please feel free to request changes @findepi
( original discussion https://github.com/trinodb/trino/pull/11191/\#discussion_r814866503 )

[grantatspothero]

Will let others review, but LGTM

[hashhar]

LGTM except URL compat "feature".

[hashhar]

Please squash fixups. Commit message could use more details, something like

Use official SingleStore JDBC driver in MemSQL connector

While the MariaDB JDBC driver works with SingleStore today it's possible
that the drivers will diverge over time and lead to subtle bugs.

The SingleStore JDBC driver exposes unsigned types differently and also
wraps TIME values outside the 00:00:00 to 23:59:59 range to lie within
the range which makes such values not round trip properly. The type
mapping is changed to perform validation similar to what the MariaDB
driver did before this change.

LGTM % comment

[ebyhr]

No changes requested. I would format like this instead of passing timeReadFunction() to memsqlTimeReadFunction() if I were you.

    (resultSet, columnIndex) -> {
        // SingleStore JDBC driver wraps values >23:59:59 or <00:00:00 to that range, so we must validate them
        verifySupportedTime(resultSet, columnIndex);
        return timeReadFunction(timeType).readLong(resultSet, columnIndex);
    },
...
    private void verifySupportedTime(ResultSet resultSet, int columnIndex)
            throws SQLException
    {
        String timeString = resultSet.getString(columnIndex);
        try {
            LocalTime.from(ISO_LOCAL_TIME.parse(timeString));
        }
        catch (DateTimeParseException e) {
            throw new IllegalStateException(format("Supported Trino TIME type range is between 00:00:00 and 23:59:59.999999 but got %s", timeString), e);
        }
    }

Regarding error message, LocalTime is internal implementation and basically unrelates to users. "Supported Trino TIME type range is between 00:00:00 and 23:59:59.999999 but got %s" or something looks better.

[leveyja]

Re: error message - thanks for that, I'll update it 👍

Re: how to compose the functions - I like the separate method you suggested (I wouldn't throw throws SQLException, but I get the idea).

Unfortunately we must create timeReadFunction(timeType) when the mapping is being created, or we'd end up throwing at a different phase of execution if this check were to fail:

checkArgument(timeType.getPrecision() <= 9, "Unsupported type precision: %s", timeType);

[ebyhr]

The max time type precision in MemSQL is 6, so we could add dedicated verification as same as the write mapping:

                TimeType timeType = createTimeType(typeHandle.getRequiredDecimalDigits());
                checkArgument(timeType.getPrecision() <= MEMSQL_DATE_TIME_MAX_PRECISION, "The max time precision in MemSQL is 6");
                return Optional.of(ColumnMapping.longMapping(
                        timeType,
                        (resultSet, columnIndex) -> {
                            // SingleStore stores incorrect results when values are outside supported range, so we must validate them
                            verifySupportedTime(resultSet.getString(columnIndex));
                            return timeReadFunction(timeType).readLong(resultSet, columnIndex);
                        },
                        timeWriteFunction(timeType.getPrecision())));

[leveyja]

I'd prefer not to add add-hoc validations like this in response to a PR comment, when the original code uses a StandardColumnMapping - if we're going to validate MemSQL time types precision are 6 rather than the current 9, we should create a PR for it.

Also, just want to highlight, because I think the point has been lost - return timeReadFunction(timeType).readLong(resultSet, columnIndex) is not in any way equivalent to return existingReadFunction.readLong(resultSet, columnIndex) - the former, which you have suggested, will throw a checkArgument at Query Execution time, rather than at Mapping creation time - this is wildly different behaviour, and would break existing tests if I were to attempt it.

[leveyja]

Removed reference to MariaDB driver in comment:

 // ~MariaDB~ SingleStore JDBC driver doesn't expose timestamp precision when connecting to MemSQL cluster

@ebyhr - let me know if I should create an Issue to enforce a memsql-specific time precision check.

The way our StandardColumnMappings are written today, that 9 is hardcoded:

    public static LongReadFunction timeReadFunction(TimeType timeType)
    {
        requireNonNull(timeType, "timeType is null");
        checkArgument(timeType.getPrecision() <= 9, "Unsupported type precision: %s", timeType);

If we don't want to duplicate all of our timeReadFunctions for every database with a smaller max precision, shouldn't the timeReadFunction have a parameter "maxTargetDbPrecision" or similar? The fact that it's hard-coded to 9 leads me to believe it's a Trino limit, and not a MemSQL one (i.e., there's no need to enforce a smaller max precision for MemSQL - but I may be wrong, the methodology behind it is not clear to me from looking at the code versus reading your suggestions).

[ebyhr]

this is wildly different behaviour, and would break existing tests if I were to attempt it.

I'm not sure which tests are you mentioning. All MemSQL tests passed with the above change in my local environment.

[leveyja]

So I wrote a test which shows what would happen - this is based on TestMySqlClient - TestMemSql??? doesn't have an equivalent which would allow me to "unit test" the code...

    @Test
    public void testColumnMappingBehaviour() {
        JdbcTypeHandle handle = new JdbcTypeHandle(Types.TIME, Optional.of("time"), Optional.of(19), Optional.of(19), Optional.empty(), Optional.empty());
        // if we "defer" creation of the `LongReadFunction`, this won't throw an exception
        ColumnMapping mapping = JDBC_CLIENT.toColumnMapping(SESSION, null, handle).get();
        try {
            // instead, it would fail here, which is not what we want
            ((LongReadFunction) mapping.getReadFunction()).readLong(null, 0);
            Assert.fail("Should thrown an Exception at mapping creation time.");
        } catch (SQLException e) {
            // expected
        }
    }

In the MySqlClient there is a check that prevents the bug being exposed:

verify(1 <= timePrecision && timePrecision <= MAX_SUPPORTED_DATE_TIME_PRECISION, "Unexpected time precision %s calculated from time column size %s", timePrecision, timeColumnSize);
called from
TimeType timeType = createTimeType(getTimePrecision(typeHandle.getRequiredColumnSize()));

e.g., - there's no test for it, and if there was, we couldn't trigger it easily, but conceptually, do you understand that we create a mapping and validate the inputs, we don't create a mapping which then creates a mapping at runtime, which could fail? (Obviously for my example it's hard to write a test to show the difference - a check on JdbcTypeHandle/TimeType (at least in the MySqlClient case) would prevent the creation of the "10" precision TimeType. But the point stands - it's not a "stylistic" choice to compose the functions. It's a mistake to create a function which creates the LongReadFunction(timeType) every time it is called to read a column from a resultset.

[hashhar]

Query Execution time, rather than at Mapping creation time

@leveyja Look at impl of ColumnMapping.longMapping. It always creates a new mapping using the "suppliers" you provide it. The method is not invoked yet - it's just a supplier.
Also JdbcClient lifecycle is per-query (per trino transaction to be more precise).

[leveyja]

    public static LongReadFunction timeReadFunction(TimeType timeType)
    {
+//        requireNonNull(timeType, "timeType is null");
+//        checkArgument(timeType.getPrecision() <= 9, "Unsupported type precision: %s", timeType);
        return (resultSet, columnIndex) -> {
+            requireNonNull(timeType, "timeType is null");      
+            checkArgument(timeType.getPrecision() <= 9, "Unsupported type precision: %s", timeType);

This is the simplest way of describing why it's not ok to call timeReadFunction(timeType) inside the outer LongReadFunction - you could create a TimeReadFunction that should have failed at MappingCreationTime(tm), and defer that exception to when the mapping is executed. If it wasn't "wrapped", it would have been created + failed. Since we're "wrapping" it, I think the checks/assertions need to be executed at mapping creation time. Moving the creation to inside the outer mapping function effectively changes the behaviour.

[ebyhr]

Please fix a type in the commit body > "todayi"

Also, could you update the below comment in MemSqlClient?
// MariaDB JDBC driver doesn't expose timestamp precision when connecting to MemSQL cluster

[hashhar]

LGTM % comment

[martint]

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

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