[SPARK-48172][SQL] Fix escaping issues in JDBC Dialects #46437

mihailom-db · 2024-05-07T12:06:13Z

What changes were proposed in this pull request?

Special case escaping for MySQL and fix issues with redundant escaping for ' character.

Why are the changes needed?

When pushing down startsWith, endsWith and contains they are converted to LIKE. This requires addition of escape characters for these expressions. Unfortunately, MySQL uses ESCAPE '\' syntax instead of ESCAPE '' which would cause errors when trying to push down.

Does this PR introduce any user-facing change?

Yes

How was this patch tested?

Tests for each existing dialect.

Was this patch authored or co-authored using generative AI tooling?

No.

yaooqinn · 2024-05-07T12:40:20Z

Existing tests.

I believe that it cannot be included under the current coverage. Can we add some new tests?

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala

sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java

mihailom-db · 2024-05-09T09:27:08Z

@cloud-fan Could you take a look at this fix?

beliefer · 2024-05-09T09:48:50Z

sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java

      switch (c) {
        case '_' -> builder.append("\\_");
        case '%' -> builder.append("\\%");
-        case '\'' -> builder.append("\\\'");


Do you see the comment of escapeSpecialCharsForLikePattern ?

Yes I do. Unfortunately, ' is not a special character that should be escaped for like expression in this way for all JDBCDialects. First red flag is that H2 had to remove this change, wouldn't we expect that the special cases of JDBC have to only add characters? Second red flag was JDBCV2Suite that actually had a problem as it is not calling visitLiteral that is implemented in JDBCDialect, but the one from V2ExpressionSQLBuilder when it was displaying the pushdown result, which is why I would presume this escape was added in the first place. We need to escape ' only when we are using pure string literals, as these literals in sql come in format of 'value'. This addition to escape ' is already done in visitLiteral and should not be done here one more time.

The input of escapeSpecialCharsForLikePattern is already a valid SQL string literal (produced by visitLiteral), so the ' is already escaped.

private[jdbc] class JDBCSQLBuilder extends V2ExpressionSQLBuilder { override def visitLiteral(literal: Literal[_]): String = { Option(literal.value()).map(v => compileValue(CatalystTypeConverters.convertToScala(v, literal.dataType())).toString) .getOrElse(super.visitLiteral(literal)) }

Stale review.

cloud-fan · 2024-05-10T06:22:49Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/H2Dialect.scala

  }

  class H2SQLBuilder extends JDBCSQLBuilder {
-    override def escapeSpecialCharsForLikePattern(str: String): String = {


I should have noticed this at the beginning... This bug is hidden because we fixed it only for H2 and we only test it with H2.

cloud-fan · 2024-05-10T06:26:13Z

...integration-tests/src/test/scala/org/apache/spark/sql/jdbc/MsSqlServerIntegrationSuite.scala

      parameters = Map("typeName" -> "sql_variant", "jdbcType" -> "-156"))
  }
+
+  test("test contains pushdown") {


do we have a parent suite for this docker JDBC test suites?

sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java

beliefer · 2024-05-11T03:19:41Z

sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java

      switch (c) {
        case '_' -> builder.append("\\_");
        case '%' -> builder.append("\\%");
-        case '\'' -> builder.append("\\\'");


beliefer · 2024-05-11T03:29:12Z

LGTM except @cloud-fan 's comment.

Special case escaping for MySQL and fix issues with redundant escaping for ' character. When pushing down startsWith, endsWith and contains they are converted to LIKE. This requires addition of escape characters for these expressions. Unfortunately, MySQL uses ESCAPE '\\' syntax instead of ESCAPE '\' which would cause errors when trying to push down. Yes Tests for each existing dialect. No. Closes #46437 from mihailom-db/SPARK-48172. Authored-by: Mihailo Milosevic <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 47006a4) Signed-off-by: Wenchen Fan <[email protected]>

cloud-fan · 2024-05-14T15:35:56Z

merged to master/3.5/3.4!

panbingkun · 2024-05-14T23:27:56Z

@mihailom-db
Is the GA failure related to this pr?

https://github.com/panbingkun/spark/actions/runs/9086615518/job/24972712440

panbingkun · 2024-05-15T01:33:24Z

From the GA's results, it seems that only MySQL is good, and everything else has problems, not only the support of data type LONGTEXT
cc @beliefer @yaooqinn @HyukjinKwon @cloud-fan

panbingkun · 2024-05-15T01:53:02Z

When I change the data type from LONGTEXT to varchar(100), it seems that there are different problems in MsSqlServerIntegrationSuite, DB2IntegrationSuite, PostgresIntegrationSuite , eg:

yaooqinn · 2024-05-15T02:07:56Z

Thank @panbingkun，I reverted this in 4ff5ca8

panbingkun · 2024-05-15T02:09:14Z

Thank @panbingkun，I reverted this in 4ff5ca8

Thanks! @yaooqinn

@mihailom-db After fix it, you can resubmit :)

This PR is a fix of #46437. The previous PR was reverted as `LONGTEXT` is not supported by all dialects. ### What changes were proposed in this pull request? Special case escaping for MySQL and fix issues with redundant escaping for ' character. New changes introduced in the fix include change `LONGTEXT` -> `VARCHAR(50)`, as well as fix for table naming in the tests. ### Why are the changes needed? When pushing down startsWith, endsWith and contains they are converted to LIKE. This requires addition of escape characters for these expressions. Unfortunately, MySQL uses ESCAPE '\' syntax instead of ESCAPE '' which would cause errors when trying to push down. ### Does this PR introduce any user-facing change? Yes ### How was this patch tested? Tests for each existing dialect. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46588 from mihailom-db/SPARK-48172. Authored-by: Mihailo Milosevic <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

This PR is a fix of #46437. The previous PR was reverted as `LONGTEXT` is not supported by all dialects. Special case escaping for MySQL and fix issues with redundant escaping for ' character. New changes introduced in the fix include change `LONGTEXT` -> `VARCHAR(50)`, as well as fix for table naming in the tests. When pushing down startsWith, endsWith and contains they are converted to LIKE. This requires addition of escape characters for these expressions. Unfortunately, MySQL uses ESCAPE '\' syntax instead of ESCAPE '' which would cause errors when trying to push down. Yes Tests for each existing dialect. No. Closes #46588 from mihailom-db/SPARK-48172. Authored-by: Mihailo Milosevic <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 9e386b4) Signed-off-by: Wenchen Fan <[email protected]>

Fix escaping issue for mysql

3140ebf

github-actions bot added the SQL label May 7, 2024

mihailom-db added 2 commits May 7, 2024 14:08

Remove unrelated changes

1fda363

Remove unrelated changes

81b6459

dongjoon-hyun previously requested changes May 7, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala Outdated Show resolved Hide resolved

mihailom-db changed the title ~~[SPARK-48172][SQL] Fix escaping issue for mysql~~ [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects May 8, 2024

mihailom-db added 5 commits May 8, 2024 08:47

Fix tests

339e8a9

Remove unused imports

b6ebebe

Fix ' escaping

4b7e3f5

Add MySQL tests

e117da9

Fix line length

dfbc210

HyukjinKwon reviewed May 8, 2024

View reviewed changes

sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java Outdated Show resolved Hide resolved

Add tests for expression pushdown to different JDBCs

9062037

milastdbx approved these changes May 9, 2024

View reviewed changes

beliefer reviewed May 9, 2024

View reviewed changes

cloud-fan reviewed May 10, 2024

View reviewed changes

sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java Outdated Show resolved Hide resolved

mihailom-db added 2 commits May 10, 2024 08:59

Move ' escaping to LiteralValue

d88fcbb

Remove unused imports

dc3c91b

beliefer approved these changes May 11, 2024

View reviewed changes

mihailom-db added 5 commits May 13, 2024 12:36

Move tests to use v2 push down

4be5786

Revert unnecessary changes

7f70b67

Fix test

13f6a81

Fix closing bracket

9d07698

conn -> connection

b3e32f5

Rename variable

45fcb60

mihailom-db requested a review from cloud-fan May 14, 2024 07:59

cloud-fan approved these changes May 14, 2024

View reviewed changes

cloud-fan closed this in 47006a4 May 14, 2024

mihailom-db mentioned this pull request May 15, 2024

[SPARK-48172][SQL] Fix escaping issues in JDBCDialects #46588

Closed

[SPARK-48172][SQL] Fix escaping issues in JDBC Dialects #46437

[SPARK-48172][SQL] Fix escaping issues in JDBC Dialects #46437

Uh oh!

Conversation

mihailom-db commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

yaooqinn commented May 7, 2024

Uh oh!

Uh oh!

Uh oh!

mihailom-db commented May 9, 2024

Uh oh!

beliefer May 9, 2024

Choose a reason for hiding this comment

Uh oh!

mihailom-db May 9, 2024

Choose a reason for hiding this comment

Uh oh!

cloud-fan May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beliefer May 11, 2024

Choose a reason for hiding this comment

Uh oh!

cloud-fan May 10, 2024

Choose a reason for hiding this comment

Uh oh!

cloud-fan May 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

beliefer May 11, 2024

Choose a reason for hiding this comment

Uh oh!

beliefer commented May 11, 2024

Uh oh!

cloud-fan commented May 14, 2024

Uh oh!

panbingkun commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panbingkun commented May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panbingkun commented May 15, 2024

Uh oh!

yaooqinn commented May 15, 2024

Uh oh!

panbingkun commented May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mihailom-db commented May 7, 2024 •

edited

Loading

cloud-fan May 10, 2024 •

edited

Loading

panbingkun commented May 14, 2024 •

edited

Loading

panbingkun commented May 15, 2024 •

edited

Loading

panbingkun commented May 15, 2024 •

edited

Loading