Skip to content

Escape comment value in JDBC connectors#14088

Merged
ebyhr merged 1 commit intotrinodb:masterfrom
ebyhr:ebi/comment-special-character
Sep 14, 2022
Merged

Escape comment value in JDBC connectors#14088
ebyhr merged 1 commit intotrinodb:masterfrom
ebyhr:ebi/comment-special-character

Conversation

@ebyhr
Copy link
Copy Markdown
Member

@ebyhr ebyhr commented Sep 10, 2022

Description

Fixes #14058

Release notes

(x) Release notes are required, with the following suggested text:

# ClickHouse
* Fix query failure when adding a column with a column comment which has special characters which require it to be escaped. ({issue}`14058`)

# ClickHouse, MariaDB, MySQL, 
* Fix query failure when creating a table with table or column comment which has special characters which require it to be escaped. ({issue}`14058`)
* Fix query failure when setting a table comment which has special characters which require it to be escaped. ({issue}`14058`)

# ClickHouse, Oracle, PostgreSQL, Redshift
* Fix query failure when setting a column comment which has special characters which require it to be escaped. ({issue}`14058`)

@cla-bot cla-bot bot added the cla-signed label Sep 10, 2022
@ebyhr ebyhr force-pushed the ebi/comment-special-character branch 2 times, most recently from c5e2466 to 93a9940 Compare September 10, 2022 05:09
@ebyhr ebyhr marked this pull request as ready for review September 10, 2022 05:09
@ebyhr ebyhr force-pushed the ebi/comment-special-character branch 2 times, most recently from 777cd15 to d2a7bb1 Compare September 10, 2022 08:00
Copy link
Copy Markdown
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.

(skimming)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not sure there is much benefit of using the prepared statement for the comment.
it's nice, but we still need to know how to escape all the other stuff (table name, column names, check constraints (in the future), etc). Escaping the COMMENT value isn't any harder that what we do today.

Copy link
Copy Markdown
Member Author

@ebyhr ebyhr Sep 12, 2022

Choose a reason for hiding this comment

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

In case of MariaDB, they have NO_BACKSLASH_ESCAPES (global & session) property. If we escape the comment without respecting the property in our side, the inserted comment might be different from what we specified in Trino DDL. Also, I guess there're other cases we are missing. That's why I introduced prepared statements in this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We control the MariaDB connection (session), so we should force NO_BACKSLASH_ESCAPES to be true.
This is important for complex predicate pushdown as well, see RewriteVarcharConstant.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reverted prepared statement change. Do you want me to force NO_BACKSLASH_ESCAPES in this PR?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can do this as a follow up (It's a "minor correctness" problem. We just don't properly support comments with backslashes.)

@ebyhr ebyhr force-pushed the ebi/comment-special-character branch 4 times, most recently from e0183d8 to fc1b94d Compare September 13, 2022 00:54
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's better to have addition of surrounding ' and escaping intra-value ' in a single place.

comment.map(PostgreSqlClient::varcharLiteral).orElse("NULL")
private static String varcharLiteral(String value) {
{
   return "'"  + value.replace("'", "''") + "'";
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

BTW this is SQL-standard behavior, it doesn't need to be implemented in this class, i.e. every conformant DB should behave like this, so it can be provided by the base-jdbc

@ebyhr ebyhr force-pushed the ebi/comment-special-character branch from 0e63591 to 697d9d6 Compare September 13, 2022 22:32
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Added this in the test

Co-Authored-By: Piotr Findeisen <piotr.findeisen@gmail.com>
@ebyhr ebyhr force-pushed the ebi/comment-special-character branch from 1c198cf to 52437d4 Compare September 14, 2022 01:07
@ebyhr ebyhr merged commit e860024 into trinodb:master Sep 14, 2022
@ebyhr ebyhr deleted the ebi/comment-special-character branch September 14, 2022 02:57
@github-actions github-actions bot added this to the 396 milestone Sep 14, 2022
@ebyhr ebyhr mentioned this pull request Sep 14, 2022
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.

Fix COMMENT value when it contains special characters

2 participants