Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 17 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
3140ebf
Fix escaping issue for mysql
mihailom-db May 7, 2024
1fda363
Remove unrelated changes
mihailom-db May 7, 2024
81b6459
Remove unrelated changes
mihailom-db May 7, 2024
339e8a9
Fix tests
mihailom-db May 8, 2024
b6ebebe
Remove unused imports
mihailom-db May 8, 2024
4b7e3f5
Fix ' escaping
mihailom-db May 8, 2024
e117da9
Add MySQL tests
mihailom-db May 8, 2024
dfbc210
Fix line length
mihailom-db May 8, 2024
9062037
Add tests for expression pushdown to different JDBCs
mihailom-db May 9, 2024
d88fcbb
Move ' escaping to LiteralValue
mihailom-db May 10, 2024
dc3c91b
Remove unused imports
mihailom-db May 10, 2024
4be5786
Move tests to use v2 push down
mihailom-db May 13, 2024
7f70b67
Revert unnecessary changes
mihailom-db May 13, 2024
13f6a81
Fix test
mihailom-db May 13, 2024
9d07698
Fix closing bracket
mihailom-db May 13, 2024
b3e32f5
conn -> connection
mihailom-db May 13, 2024
45fcb60
Rename variable
mihailom-db May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...r-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/DB2IntegrationSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,12 @@ class DB2IntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCTest {
connection.prepareStatement(
"CREATE TABLE employee (dept INTEGER, name VARCHAR(10), salary DECIMAL(20, 2), bonus DOUBLE)")
.executeUpdate()
connection.prepareStatement(
s"""CREATE TABLE pattern_testing_table (
|pattern_testing_col LONGTEXT
|)
""".stripMargin
).executeUpdate()
}

override def testUpdateColumnType(tbl: String): Unit = {
Expand Down
11 changes: 11 additions & 0 deletions ...tion-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/DockerJDBCIntegrationV2Suite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,17 @@ abstract class DockerJDBCIntegrationV2Suite extends DockerJDBCIntegrationSuite {
.executeUpdate()
connection.prepareStatement("INSERT INTO employee VALUES (6, 'jen', 12000, 1200)")
.executeUpdate()

connection.prepareStatement(
s"""
|INSERT INTO pattern_testing_table VALUES
|('special_character_quote\\'_present'),
|('special_character_quote_not_present'),
|('special_character_percent%_present'),
|('special_character_percent_not_present'),
|('special_character_underscore_present'),
|('special_character_underscorenot_present')
""".stripMargin).executeUpdate()
}

def tablePreparation(connection: Connection): Unit
Expand Down
6 changes: 6 additions & 0 deletions ...ation-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,12 @@ class MsSqlServerIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JD
connection.prepareStatement(
"CREATE TABLE employee (dept INT, name VARCHAR(32), salary NUMERIC(20, 2), bonus FLOAT)")
.executeUpdate()
connection.prepareStatement(
s"""CREATE TABLE pattern_testing_table (
|pattern_testing_col LONGTEXT
|)
""".stripMargin
).executeUpdate()
}

override def notSupportsTableComment: Boolean = true
Expand Down
6 changes: 6 additions & 0 deletions ...integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MySQLIntegrationSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,12 @@ class MySQLIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCTest
connection.prepareStatement(
"CREATE TABLE employee (dept INT, name VARCHAR(32), salary DECIMAL(20, 2)," +
" bonus DOUBLE)").executeUpdate()
connection.prepareStatement(
s"""CREATE TABLE pattern_testing_table (
|pattern_testing_col LONGTEXT
|)
""".stripMargin
).executeUpdate()
}

override def testUpdateColumnType(tbl: String): Unit = {
Expand Down
6 changes: 6 additions & 0 deletions ...ntegration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,12 @@ class OracleIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCTes
connection.prepareStatement(
"CREATE TABLE employee (dept NUMBER(32), name VARCHAR2(32), salary NUMBER(20, 2)," +
" bonus BINARY_DOUBLE)").executeUpdate()
connection.prepareStatement(
s"""CREATE TABLE pattern_testing_table (
|pattern_testing_col LONGTEXT
|)
""".stripMargin
).executeUpdate()
}

override def testUpdateColumnType(tbl: String): Unit = {
Expand Down
6 changes: 6 additions & 0 deletions ...egration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,12 @@ class PostgresIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCT
connection.prepareStatement(
"CREATE TABLE employee (dept INTEGER, name VARCHAR(32), salary NUMERIC(20, 2)," +
" bonus double precision)").executeUpdate()
connection.prepareStatement(
s"""CREATE TABLE pattern_testing_table (
|pattern_testing_col LONGTEXT
|)
""".stripMargin
).executeUpdate()
}

override def testUpdateColumnType(tbl: String): Unit = {
Expand Down
229 changes: 229 additions & 0 deletions ...tor/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/V2JDBCTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,235 @@ private[v2] trait V2JDBCTest extends SharedSparkSession with DockerIntegrationFu
assert(scan.schema.names.sameElements(Seq(col)))
}

test("SPARK-48172: Test CONTAINS") {
val df1 = spark.sql(
s"""
|SELECT * FROM $catalogName.pattern_testing_table
|WHERE contains(pattern_testing_col, 'quote\\'')""".stripMargin)
df1.explain("formatted")
val rows1 = df1.collect()
assert(rows1.length === 1)
assert(rows1(0).getString(0) === "special_character_quote'_present")

val df2 = spark.sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE contains(pattern_testing_col, 'percent%')""".stripMargin)
val rows2 = df2.collect()
assert(rows2.length === 1)
assert(rows2(0).getString(0) === "special_character_percent%_present")

val df3 = spark.
sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE contains(pattern_testing_col, 'underscore_')""".stripMargin)
val rows3 = df3.collect()
assert(rows3.length === 1)
assert(rows3(0).getString(0) === "special_character_underscore_present")

val df4 = spark.
sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE contains(pattern_testing_col, 'character')
|ORDER BY pattern_testing_col""".stripMargin)
val rows4 = df4.collect()
assert(rows4.length === 1)
assert(rows4(0).getString(0) === "special_character_percent%_present")
assert(rows4(1).getString(0) === "special_character_percent_not_present")
assert(rows4(2).getString(0) === "special_character_quote'_present")
assert(rows4(3).getString(0) === "special_character_quote_not_present")
assert(rows4(4).getString(0) === "special_character_underscore_present")
assert(rows4(5).getString(0) === "special_character_underscorenot_present")
}

test("SPARK-48172: Test ENDSWITH") {
val df1 = spark.sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE endswith(pattern_testing_col, 'quote\\'_present')""".stripMargin)
val rows1 = df1.collect()
assert(rows1.length === 1)
assert(rows1(0).getString(0) === "special_character_quote'_present")

val df2 = spark.sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE endswith(pattern_testing_col, 'percent%_present')""".stripMargin)
val rows2 = df2.collect()
assert(rows2.length === 1)
assert(rows2(0).getString(0) === "special_character_percent%_present")

val df3 = spark.
sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE endswith(pattern_testing_col, 'underscore_present')""".stripMargin)
val rows3 = df3.collect()
assert(rows3.length === 1)
assert(rows3(0).getString(0) === "special_character_underscore_present")

val df4 = spark.
sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE endswith(pattern_testing_col, 'present')
|ORDER BY pattern_testing_col""".stripMargin)
val rows4 = df4.collect()
assert(rows4.length === 1)
assert(rows4(0).getString(0) === "special_character_percent%_present")
assert(rows4(1).getString(0) === "special_character_percent_not_present")
assert(rows4(2).getString(0) === "special_character_quote'_present")
assert(rows4(3).getString(0) === "special_character_quote_not_present")
assert(rows4(4).getString(0) === "special_character_underscore_present")
assert(rows4(5).getString(0) === "special_character_underscorenot_present")
}

test("SPARK-48172: Test STARTSWITH") {
val df1 = spark.sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE startswith(pattern_testing_col, 'special_character_quote\\'')""".stripMargin)
val rows1 = df1.collect()
assert(rows1.length === 1)
assert(rows1(0).getString(0) === "special_character_quote'_present")

val df2 = spark.sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE startswith(pattern_testing_col, 'special_character_percent%')""".stripMargin)
val rows2 = df2.collect()
assert(rows2.length === 1)
assert(rows2(0).getString(0) === "special_character_percent%_present")

val df3 = spark.
sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE startswith(pattern_testing_col, 'special_character_underscore_')""".stripMargin)
val rows3 = df3.collect()
assert(rows3.length === 1)
assert(rows3(0).getString(0) === "special_character_underscore_present")

val df4 = spark.
sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE startswith(pattern_testing_col, 'special_character')
|ORDER BY pattern_testing_col""".stripMargin)
val rows4 = df4.collect()
assert(rows4.length === 1)
assert(rows4(0).getString(0) === "special_character_percent%_present")
assert(rows4(1).getString(0) === "special_character_percent_not_present")
assert(rows4(2).getString(0) === "special_character_quote'_present")
assert(rows4(3).getString(0) === "special_character_quote_not_present")
assert(rows4(4).getString(0) === "special_character_underscore_present")
assert(rows4(5).getString(0) === "special_character_underscorenot_present")
}

test("SPARK-48172: Test LIKE") {
// this one should map to contains
val df1 = spark.sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE pattern_testing_col LIKE '%quote\\'%'""".stripMargin)
val rows1 = df1.collect()
assert(rows1.length === 1)
assert(rows1(0).getString(0) === "special_character_quote'_present")

val df2 = spark.sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE pattern_testing_col LIKE '%percent\\%%'""".stripMargin)
val rows2 = df2.collect()
assert(rows2.length === 1)
assert(rows2(0).getString(0) === "special_character_percent%_present")

val df3 = spark.
sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE pattern_testing_col LIKE '%underscore\\_%'""".stripMargin)
val rows3 = df3.collect()
assert(rows3.length === 1)
assert(rows3(0).getString(0) === "special_character_underscore_present")

val df4 = spark.
sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE pattern_testing_col LIKE '%character%'
|ORDER BY pattern_testing_col""".stripMargin)
val rows4 = df4.collect()
assert(rows4.length === 1)
assert(rows4(0).getString(0) === "special_character_percent%_present")
assert(rows4(1).getString(0) === "special_character_percent_not_present")
assert(rows4(2).getString(0) === "special_character_quote'_present")
assert(rows4(3).getString(0) === "special_character_quote_not_present")
assert(rows4(4).getString(0) === "special_character_underscore_present")
assert(rows4(5).getString(0) === "special_character_underscorenot_present")

// map to startsWith
// this one should map to contains
val df5 = spark.sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE pattern_testing_col LIKE 'special_character_quote\\'%'""".stripMargin)
val rows5 = df5.collect()
assert(rows5.length === 1)
assert(rows5(0).getString(0) === "special_character_quote'_present")

val df6 = spark.sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE pattern_testing_col LIKE 'special_character_percent\\%%'""".stripMargin)
val rows6 = df6.collect()
assert(rows6.length === 1)
assert(rows6(0).getString(0) === "special_character_percent%_present")

val df7 = spark.
sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE pattern_testing_col LIKE 'special_character_underscore\\_%'""".stripMargin)
val rows7 = df7.collect()
assert(rows7.length === 1)
assert(rows7(0).getString(0) === "special_character_underscore_present")

val df8 = spark.
sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE pattern_testing_col LIKE 'special_character%'
|ORDER BY pattern_testing_col""".stripMargin)
val rows8 = df8.collect()
assert(rows8.length === 1)
assert(rows8(0).getString(0) === "special_character_percent%_present")
assert(rows8(1).getString(0) === "special_character_percent_not_present")
assert(rows8(2).getString(0) === "special_character_quote'_present")
assert(rows8(3).getString(0) === "special_character_quote_not_present")
assert(rows8(4).getString(0) === "special_character_underscore_present")
assert(rows8(5).getString(0) === "special_character_underscorenot_present")
// map to endsWith
// this one should map to contains
val df9 = spark.sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE pattern_testing_col LIKE '%quote\\'_present'""".stripMargin)
val rows9 = df9.collect()
assert(rows9.length === 1)
assert(rows9(0).getString(0) === "special_character_quote'_present")

val df10 = spark.sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE pattern_testing_col LIKE '%percent\\%_present'""".stripMargin)
val rows10 = df10.collect()
assert(rows10.length === 1)
assert(rows10(0).getString(0) === "special_character_percent%_present")

val df11 = spark.
sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE pattern_testing_col LIKE '%underscore\\_present'""".stripMargin)
val rows11 = df11.collect()
assert(rows11.length === 1)
assert(rows11(0).getString(0) === "special_character_underscore_present")

val df12 = spark.
sql(
s"""SELECT * FROM $catalogName.pattern_testing_table
|WHERE pattern_testing_col LIKE '%present' ORDER BY pattern_testing_col""".stripMargin)
val rows12 = df12.collect()
assert(rows12.length === 1)
assert(rows12(0).getString(0) === "special_character_percent%_present")
assert(rows12(1).getString(0) === "special_character_percent_not_present")
assert(rows12(2).getString(0) === "special_character_quote'_present")
assert(rows12(3).getString(0) === "special_character_quote_not_present")
assert(rows12(4).getString(0) === "special_character_underscore_present")
assert(rows12(5).getString(0) === "special_character_underscorenot_present")
}

test("SPARK-37038: Test TABLESAMPLE") {
if (supportsTableSample) {
withTable(s"$catalogName.new_table") {
Expand Down
1 change: 0 additions & 1 deletion sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ protected String escapeSpecialCharsForLikePattern(String str) {
switch (c) {
case '_' -> builder.append("\\_");
case '%' -> builder.append("\\%");
case '\'' -> builder.append("\\\'");
Copy link
Contributor

@beliefer beliefer May 9, 2024

Choose a reason for hiding this comment

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

Do you see the comment of escapeSpecialCharsForLikePattern ?

Copy link
Contributor Author

@mihailom-db mihailom-db May 9, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@cloud-fan cloud-fan May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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))
    }

Copy link
Contributor

@beliefer beliefer May 11, 2024

Choose a reason for hiding this comment

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

Got it.

default -> builder.append(c);
}
}
Expand Down
4 changes: 3 additions & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

package org.apache.spark.sql.connector.expressions

import org.apache.commons.lang3.StringUtils

import org.apache.spark.SparkException
import org.apache.spark.sql.catalyst
import org.apache.spark.sql.catalyst.parser.CatalystSqlParser
Expand Down Expand Up @@ -388,7 +390,7 @@ private[sql] object HoursTransform {
private[sql] final case class LiteralValue[T](value: T, dataType: DataType) extends Literal[T] {
override def toString: String = {
if (dataType.isInstanceOf[StringType]) {
s"'$value'"
s"'${StringUtils.replace(s"$value", "'", "''")}'"
} else {
s"$value"
}
Expand Down
7 changes: 0 additions & 7 deletions sql/core/src/main/scala/org/apache/spark/sql/jdbc/H2Dialect.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,13 +259,6 @@ private[sql] case class H2Dialect() extends JdbcDialect {
}

class H2SQLBuilder extends JDBCSQLBuilder {
override def escapeSpecialCharsForLikePattern(str: String): String = {
Copy link
Contributor

@cloud-fan cloud-fan May 10, 2024

Choose a reason for hiding this comment

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

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.

str.map {
case '_' => "\\_"
case '%' => "\\%"
case c => c.toString
}.mkString
}

override def visitAggregateFunction(
funcName: String, isDistinct: Boolean, inputs: Array[String]): String =
Expand Down
15 changes: 15 additions & 0 deletions sql/core/src/main/scala/org/apache/spark/sql/jdbc/MySQLDialect.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,21 @@ private case class MySQLDialect() extends JdbcDialect with SQLConfHelper {
}
}

override def visitStartsWith(l: String, r: String): String = {
val value = r.substring(1, r.length() - 1)
s"$l LIKE '${escapeSpecialCharsForLikePattern(value)}%' ESCAPE '\\\\'"
}

override def visitEndsWith(l: String, r: String): String = {
val value = r.substring(1, r.length() - 1)
s"$l LIKE '%${escapeSpecialCharsForLikePattern(value)}' ESCAPE '\\\\'"
}

override def visitContains(l: String, r: String): String = {
val value = r.substring(1, r.length() - 1)
s"$l LIKE '%${escapeSpecialCharsForLikePattern(value)}%' ESCAPE '\\\\'"
}

override def visitAggregateFunction(
funcName: String, isDistinct: Boolean, inputs: Array[String]): String =
if (isDistinct && distinctUnsupportedAggregateFunctions.contains(funcName)) {
Expand Down
Loading