Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make MySQL JDBC driver strictly follow JDBC #19292

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hashhar
Copy link
Member

@hashhar hashhar commented Oct 6, 2023

pedantic configuration property changes the MySQL JDBC driver to more strictly follow the JDBC specification.

The one behaviour we're most interested in is handling of quoted identifiers. Without this property if an identifier like ab``c is left as it is instead of being properly quoted like ab````c.

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

FYI @findepi

@cla-bot cla-bot bot added the cla-signed label Oct 6, 2023
@hashhar
Copy link
Member Author

hashhar commented Oct 6, 2023

let's see what blows up and then see if we can add a test for the problematic identifiers.

Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

i searched MariaDB docs for same property, but didn't find it.
Didn't check the code though.

Didn't check other MySQL-alike, the Singlestore

@findepi
Copy link
Member

findepi commented Oct 6, 2023

I think we could test cover this, using table names with backticks.

pedantic configuration property changes the MySQL JDBC driver to more
strictly follow the JDBC specification.

The one behaviour we're most interested in is handling of quoted
identifiers. Without this property if an identifier like `ab``c` is left
as it is instead of being properly quoted like ```ab````c```.
@hashhar
Copy link
Member Author

hashhar commented Sep 15, 2024

doesn't matter with or without pedantic. Seems pedantic was added to fix getcolumns for tables name like `back`quote` but only when called with a null table name which isn't something we ever do.

While trying to add a test however I noticed a different bug which I've reported to MySQL folks - it's a bug in their driver.

getTables doesn't return tables of the form `back``quoted` since it unquotes the tableNamePattern from `back``quoted` to back``quoted.

So if your MySQL has a table named like that Trino will always think it doesn't exist since we use getTables to create table handle for example.

Follow along at https://bugs.mysql.com/bug.php?id=116113

@hashhar
Copy link
Member Author

hashhar commented Sep 15, 2024

Here's the test I used to find this:

    @Test
    public void testQuotedIdentifiers()
    {
        assertUpdate("""
                CREATE TABLE tpch."`back``quoted`" ("`back``quoted`" BIGINT)""");
        assertThat(computeActual("SHOW CREATE TABLE tpch.\"`back``quoted`\"").getOnlyValue())
                .isEqualTo("""
                        CREATE TABLE mysql.tpch."`back``quoted`" (
                           "`back``quoted`" bigint
                        )""");

        assertUpdate("INSERT INTO tpch.\"`back``quoted`\" VALUES (1)", 1);
        assertThat(query("SELECT * FROM \"`back``quoted`\""))
                .matches("VALUES (BIGINT '1')");

        assertQuery("SHOW COLUMNS FROM tpch.\"`back``quoted`\"", "VALUES ('`back``quoted`', 'bigint', '', '')");
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants