You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "mihailom-db (via GitHub)" <gi...@apache.org> on 2024/05/07 12:06:16 UTC
[PR] [SPARK-48172][SQL] Fix escaping issue for mysql [spark]
mihailom-db opened a new pull request, #46437:
URL: https://github.com/apache/spark/pull/46437
### 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?
Existing tests.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "mihailom-db (via GitHub)" <gi...@apache.org>.
mihailom-db commented on PR #46437:
URL: https://github.com/apache/spark/pull/46437#issuecomment-2102293937
@cloud-fan Could you take a look at this fix?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #46437:
URL: https://github.com/apache/spark/pull/46437#discussion_r1597343529
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java:
##########
@@ -65,7 +68,6 @@ protected String escapeSpecialCharsForLikePattern(String str) {
switch (c) {
case '_' -> builder.append("\\_");
case '%' -> builder.append("\\%");
- case '\'' -> builder.append("\\\'");
Review Comment:
Got it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #46437:
URL: https://github.com/apache/spark/pull/46437#issuecomment-2111421746
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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #46437:
URL: https://github.com/apache/spark/pull/46437#discussion_r1596314679
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java:
##########
@@ -65,7 +68,6 @@ protected String escapeSpecialCharsForLikePattern(String str) {
switch (c) {
case '_' -> builder.append("\\_");
case '%' -> builder.append("\\%");
- case '\'' -> builder.append("\\\'");
Review Comment:
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))
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #46437:
URL: https://github.com/apache/spark/pull/46437#issuecomment-2105511312
LGTM except @cloud-fan 's comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #46437: [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects
URL: https://github.com/apache/spark/pull/46437
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #46437:
URL: https://github.com/apache/spark/pull/46437#discussion_r1596315249
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/MsSqlServerIntegrationSuite.scala:
##########
@@ -493,4 +511,182 @@ class MsSqlServerIntegrationSuite extends DockerJDBCIntegrationSuite {
errorClass = "UNRECOGNIZED_SQL_TYPE",
parameters = Map("typeName" -> "sql_variant", "jdbcType" -> "-156"))
}
+
+ test("test contains pushdown") {
Review Comment:
do we have a parent suite for this docker JDBC test suites?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #46437:
URL: https://github.com/apache/spark/pull/46437#issuecomment-2110549856
merged to master/3.5/3.4!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #46437:
URL: https://github.com/apache/spark/pull/46437#issuecomment-2111321996
Is the GA failure related to this pr?
<img width="877" alt="image" src="https://github.com/apache/spark/assets/15246973/cef579ab-37c6-4bef-bfb4-b23f7b3d59df">
https://github.com/panbingkun/spark/actions/runs/9086615518/job/24972712440
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #46437:
URL: https://github.com/apache/spark/pull/46437#discussion_r1596314679
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java:
##########
@@ -65,7 +68,6 @@ protected String escapeSpecialCharsForLikePattern(String str) {
switch (c) {
case '_' -> builder.append("\\_");
case '%' -> builder.append("\\%");
- case '\'' -> builder.append("\\\'");
Review Comment:
The input of `escapeSpecialCharsForLikePattern` is already a valid SQL string literal, so the `'` is already escaped.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issue for mysql [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #46437:
URL: https://github.com/apache/spark/pull/46437#issuecomment-2098314295
> Existing tests.
I believe that it cannot be included under the current coverage. Can we add some new tests?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #46437:
URL: https://github.com/apache/spark/pull/46437#discussion_r1595229250
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java:
##########
@@ -65,7 +68,6 @@ protected String escapeSpecialCharsForLikePattern(String str) {
switch (c) {
case '_' -> builder.append("\\_");
case '%' -> builder.append("\\%");
- case '\'' -> builder.append("\\\'");
Review Comment:
Do you see the comment of `escapeSpecialCharsForLikePattern` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #46437:
URL: https://github.com/apache/spark/pull/46437#issuecomment-2111449319
Thank @panbingkun，I reverted this in https://github.com/apache/spark/commit/4ff5ca81cffbd1940c864144ca8fbba54b605e4e
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46437:
URL: https://github.com/apache/spark/pull/46437#discussion_r1594810430
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java:
##########
@@ -169,7 +171,16 @@ yield visitBinaryArithmetic(
}
protected String visitLiteral(Literal<?> literal) {
- return literal.toString();
+ String litString = literal.toString();
+ if (literal.dataType() instanceof StringType) {
+ return "'"
+ + StringUtils.replace(
Review Comment:
Can we stick to 2 spaces? See "Code style guide" in https://spark.apache.org/contributing.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "mihailom-db (via GitHub)" <gi...@apache.org>.
mihailom-db commented on code in PR #46437:
URL: https://github.com/apache/spark/pull/46437#discussion_r1595337990
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java:
##########
@@ -65,7 +68,6 @@ protected String escapeSpecialCharsForLikePattern(String str) {
switch (c) {
case '_' -> builder.append("\\_");
case '%' -> builder.append("\\%");
- case '\'' -> builder.append("\\\'");
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issue for mysql [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #46437:
URL: https://github.com/apache/spark/pull/46437#discussion_r1592733398
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -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, "'", "\\'")
Review Comment:
To @mihailom-db , I believe this is a breaking change which we should not apply to all JDBCDialect.
If you want to fix something in MySQL, could you isolate it to MySQL only?
> [SPARK-48172][SQL] Fix escaping issue for mysql
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #46437:
URL: https://github.com/apache/spark/pull/46437#discussion_r1596316349
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java:
##########
@@ -169,7 +171,15 @@ yield visitBinaryArithmetic(
}
protected String visitLiteral(Literal<?> literal) {
- return literal.toString();
+ String litString = literal.toString();
+ if (literal.dataType() instanceof StringType) {
+ return "'"
+ + StringUtils.replace(litString.substring(1, litString.length() - 1),
Review Comment:
shall we do this change in `LiteralValue.toString`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #46437:
URL: https://github.com/apache/spark/pull/46437#discussion_r1596312241
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/H2Dialect.scala:
##########
@@ -259,13 +259,6 @@ private[sql] case class H2Dialect() extends JdbcDialect {
}
class H2SQLBuilder extends JDBCSQLBuilder {
- override def escapeSpecialCharsForLikePattern(str: String): String = {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #46437:
URL: https://github.com/apache/spark/pull/46437#issuecomment-2111450233
> Thank @panbingkun，I reverted this in [4ff5ca8](https://github.com/apache/spark/commit/4ff5ca81cffbd1940c864144ca8fbba54b605e4e)
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48172][SQL] Fix escaping issues in JDBC Dialects [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #46437:
URL: https://github.com/apache/spark/pull/46437#issuecomment-2111438207
When I change the data type from `LONGTEXT` to `varchar(100)`, it seems that there are different problems in `MsSqlServerIntegrationSuite`, `DB2IntegrationSuite`, `PostgresIntegrationSuite `, eg:
<img width="881" alt="image" src="https://github.com/apache/spark/assets/15246973/bbd56f02-0ab0-470d-8f82-523bff3110b8">
<img width="737" alt="image" src="https://github.com/apache/spark/assets/15246973/39e2a47c-3337-4653-9acb-988e9576ef99">
<img width="836" alt="image" src="https://github.com/apache/spark/assets/15246973/7fa750bc-0299-44fe-a7ca-e1151d8ae6b8">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org