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
mihailomilosevic2001 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 3 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
3140ebf
Fix escaping issue for mysql
mihailomilosevic2001 May 7, 2024
1fda363
Remove unrelated changes
mihailomilosevic2001 May 7, 2024
81b6459
Remove unrelated changes
mihailomilosevic2001 May 7, 2024
339e8a9
Fix tests
mihailomilosevic2001 May 8, 2024
b6ebebe
Remove unused imports
mihailomilosevic2001 May 8, 2024
4b7e3f5
Fix ' escaping
mihailomilosevic2001 May 8, 2024
e117da9
Add MySQL tests
mihailomilosevic2001 May 8, 2024
dfbc210
Fix line length
mihailomilosevic2001 May 8, 2024
9062037
Add tests for expression pushdown to different JDBCs
mihailomilosevic2001 May 9, 2024
d88fcbb
Move ' escaping to LiteralValue
mihailomilosevic2001 May 10, 2024
dc3c91b
Remove unused imports
mihailomilosevic2001 May 10, 2024
4be5786
Move tests to use v2 push down
mihailomilosevic2001 May 13, 2024
7f70b67
Revert unnecessary changes
mihailomilosevic2001 May 13, 2024
13f6a81
Fix test
mihailomilosevic2001 May 13, 2024
9d07698
Fix closing bracket
mihailomilosevic2001 May 13, 2024
b3e32f5
conn -> connection
mihailomilosevic2001 May 13, 2024
45fcb60
Rename variable
mihailomilosevic2001 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
5 changes: 3 additions & 2 deletions 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 @@ -22,6 +22,8 @@
import java.util.Map;
import java.util.StringJoiner;

import org.apache.commons.lang3.StringUtils;

import org.apache.spark.SparkIllegalArgumentException;
import org.apache.spark.SparkUnsupportedOperationException;
import org.apache.spark.sql.connector.expressions.Cast;
Expand Down Expand Up @@ -65,7 +67,6 @@ protected String escapeSpecialCharsForLikePattern(String str) {
switch (c) {
case '_' -> builder.append("\\_");
case '%' -> builder.append("\\%");
case '\'' -> builder.append("\\\'");

@beliefer beliefer May 9, 2024

Copy link
Copy Markdown
Contributor

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 ?

@mihailomilosevic2001 mihailomilosevic2001 May 9, 2024

Copy link
Copy Markdown
Contributor Author

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.

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

Copy link
Copy Markdown
Contributor

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

@beliefer beliefer May 11, 2024

Copy link
Copy Markdown
Contributor

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 Expand Up @@ -169,7 +170,7 @@ yield visitBinaryArithmetic(
}

protected String visitLiteral(Literal<?> literal) {
return literal.toString();
return StringUtils.replace(literal.toString(), "'", "\\'");
}

protected String visitNamedReference(NamedReference namedRef) {
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 = {

@cloud-fan cloud-fan May 10, 2024

Copy link
Copy Markdown
Contributor

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
2 changes: 1 addition & 1 deletion sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ abstract class JdbcDialect extends Serializable with Logging {
*/
@Since("2.3.0")
protected[jdbc] def escapeSql(value: String): String =
if (value == null) null else StringUtils.replace(value, "'", "''")
if (value == null) null else StringUtils.replace(value, "'", "\\'")
Comment thread
mihailomilosevic2001 marked this conversation as resolved.
Outdated

/**
* Converts value to SQL expression.
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