You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "stefanbuk-db (via GitHub)" <gi...@apache.org> on 2024/03/18 12:55:38 UTC
[PR] [WIP][SPARK-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
stefanbuk-db opened a new pull request, #45564:
URL: https://github.com/apache/spark/pull/45564
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
In this PR, I propose a change in SQLQuery builder of MsSqlServer dialect. I override build method to check for LIKE operator in binary comparisons and throw exception if encountered.
### Why are the changes needed?
MsSqlServer syntax prevents LIKE operator in any binary comparison. It was possible to construct Spark query that would generate this situation in MsSqlServer and engine would throw syntax exception. This PR solves this bug.
### Does this PR introduce _any_ user-facing change?
Yes, user will not encounter syntax exception in MsSqlServer when writing these queries.
### How was this patch tested?
By running a unit test in MsSqlServerIntegrationSuite
### 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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1535168006
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala:
##########
@@ -86,6 +87,31 @@ private object MsSqlServerDialect extends JdbcDialect {
case "STDDEV_SAMP" => "STDEV"
case _ => super.dialectFunctionName(funcName)
}
+
+ override def build(expr: Expression): String = {
+ def isChildBooleanExpression(child: Expression): Boolean = child match {
+ case _: Predicate => true
+ case _ => false
+ }
+
+ // MsSqlServer does not support boolean comparison using standard comparison operators
+ // We shouldn't propagate these queries to MsSqlServer
+ expr match {
+ case e: Predicate =>
+ e.name() match {
+ case "=" | "<>" | "<=>" | "<" | "<=" | ">" | ">=" =>
+ if (isChildBooleanExpression(e.children()(0))
+ || isChildBooleanExpression(e.children()(1))) {
+ super.visitUnexpectedExpr(expr)
+ } else {
+ super.build(expr)
+ }
+ case _ => super.build(expr)
+ }
+ case _ =>
+ super.build(expr)
+ }
+ }
Review Comment:
Thanks for suggestion!
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1532150421
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala:
##########
@@ -86,6 +86,35 @@ private object MsSqlServerDialect extends JdbcDialect {
case "STDDEV_SAMP" => "STDEV"
case _ => super.dialectFunctionName(funcName)
}
+
+ override def build(expr: Expression): String = {
+ def isChildLikeExpression(child: Expression): Boolean = child match {
+ case gse: GeneralScalarExpression =>
+ gse.name() match {
+ case "STARTS_WITH" | "ENDS_WITH" | "CONTAINS" => true
+ case _ => false
+ }
+ case _ => false
+ }
+
+ // MsSqlServer does not support boolean comparison using standard comparison operators
+ // LIKE operator has a boolean result, hence we don't propagate these queries to MsSqlServer
+ expr match {
+ case e: GeneralScalarExpression =>
+ e.name() match {
+ case "=" | "<>" | "<=>" | "<" | "<=" | ">" | ">=" =>
Review Comment:
Problem with overwriting `visitBinaryComparison` is that it takes strings as parameters. This leaves us with not much information about child expressions. Parsing these strings could be very complex and costly, so overriding this function is not the best option here.
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala:
##########
@@ -86,6 +86,35 @@ private object MsSqlServerDialect extends JdbcDialect {
case "STDDEV_SAMP" => "STDEV"
case _ => super.dialectFunctionName(funcName)
}
+
+ override def build(expr: Expression): String = {
Review Comment:
See comment above.
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #45564:
URL: https://github.com/apache/spark/pull/45564#issuecomment-2005644181
> MsSqlServer syntax prevents LIKE operator in any binary comparison.
Can we have a reference and some comments both in the PR desc and related code changes?
> Yes, user will not encounter syntax exception in MsSqlServer when writing these queries.
Can we make some examples for `these queries` in the PR desc?
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1535170433
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala:
##########
@@ -125,4 +125,20 @@ class MsSqlServerIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JD
},
errorClass = "_LEGACY_ERROR_TEMP_2271")
}
+
+ test("SPARK-47440: SQLServer does not support boolean expression in binary comparison") {
+ val df1 = sql(s"SELECT name FROM " +
+ s"$catalogName.employee WHERE ((name LIKE 'am%') = (name LIKE '%y'))")
+ assert(df1.collect().length == 4)
+
+ val df2 = sql(s"SELECT name FROM " +
+ s"$catalogName.employee " +
+ s"WHERE ((name NOT LIKE 'am%') = (name NOT LIKE '%y'))")
+ assert(df2.collect().length == 4)
+
+ val df3 = sql(s"SELECT name FROM " +
Review Comment:
```suggestion
val df3 = sql("SELECT name FROM " +
```
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1531826271
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala:
##########
@@ -86,6 +86,35 @@ private object MsSqlServerDialect extends JdbcDialect {
case "STDDEV_SAMP" => "STDEV"
case _ => super.dialectFunctionName(funcName)
}
+
+ override def build(expr: Expression): String = {
+ def isChildLikeExpression(child: Expression): Boolean = child match {
+ case gse: GeneralScalarExpression =>
+ gse.name() match {
+ case "STARTS_WITH" | "ENDS_WITH" | "CONTAINS" => true
+ case _ => false
+ }
+ case _ => false
+ }
+
+ // MsSqlServer does not support boolean comparison using standard comparison operators
+ // LIKE operator has a boolean result, hence we don't propagate these queries to MsSqlServer
+ expr match {
+ case e: GeneralScalarExpression =>
+ e.name() match {
+ case "=" | "<>" | "<=>" | "<" | "<=" | ">" | ">=" =>
Review Comment:
Please overwrite `visitBinaryComparison`
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1532152570
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala:
##########
@@ -125,4 +125,10 @@ class MsSqlServerIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JD
},
errorClass = "_LEGACY_ERROR_TEMP_2271")
}
+
+ test("SPARK-47440: SQLServer does not support LIKE in binary comparison") {
+ val df = sql(s"SELECT name FROM " +
+ s"$catalogName.employee WHERE ((name LIKE 'am%') = (name LIKE '%y'))")
Review Comment:
Added support for multiple logical operators, also added NOT test case that now passes.
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45564:
URL: https://github.com/apache/spark/pull/45564#issuecomment-2015258318
Thank you all!
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1531775266
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala:
##########
@@ -125,4 +125,10 @@ class MsSqlServerIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JD
},
errorClass = "_LEGACY_ERROR_TEMP_2271")
}
+
+ test("SPARK-47440: SQLServer does not support LIKE in binary comparison") {
+ val df = sql(s"SELECT name FROM " +
+ s"$catalogName.employee WHERE ((name LIKE 'am%') = (name LIKE '%y'))")
Review Comment:
We can fix it together
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "milastdbx (via GitHub)" <gi...@apache.org>.
milastdbx commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1529107460
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala:
##########
@@ -86,6 +87,34 @@ private object MsSqlServerDialect extends JdbcDialect {
case "STDDEV_SAMP" => "STDEV"
case _ => super.dialectFunctionName(funcName)
}
+
+ override def build(expr: Expression): String = {
+ def isChildLikeExpression(child: Expression): Boolean = child match {
+ case gse: GeneralScalarExpression =>
+ gse.name() match {
+ case "STARTS_WITH" | "ENDS_WITH" | "CONTAINS" => true
+ case _ => false
+ }
+ case _ => false
+ }
+
+ expr match {
+ case e: GeneralScalarExpression =>
+ e.name() match {
+ case "=" | "<>" | "<=>" | "<" | "<=" | ">" | ">=" =>
+ if (isChildLikeExpression(e.children()(0))
+ || isChildLikeExpression(e.children()(1))) {
+ throw new SparkUnsupportedOperationException(
Review Comment:
why dont we just call `visitUnexpectedExpr` of parent class ?
We do not propagate these errors to users, hence I think making error class for it is overkill
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1533460412
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala:
##########
@@ -86,6 +86,35 @@ private object MsSqlServerDialect extends JdbcDialect {
case "STDDEV_SAMP" => "STDEV"
case _ => super.dialectFunctionName(funcName)
}
+
+ override def build(expr: Expression): String = {
+ def isChildLikeExpression(child: Expression): Boolean = child match {
+ case gse: GeneralScalarExpression =>
+ gse.name() match {
+ case "STARTS_WITH" | "ENDS_WITH" | "CONTAINS" => true
+ case _ => false
+ }
+ case _ => false
+ }
+
+ // MsSqlServer does not support boolean comparison using standard comparison operators
+ // LIKE operator has a boolean result, hence we don't propagate these queries to MsSqlServer
+ expr match {
+ case e: GeneralScalarExpression =>
+ e.name() match {
+ case "=" | "<>" | "<=>" | "<" | "<=" | ">" | ">=" =>
Review Comment:
Shall we overwrite `visitBinaryComparison` and convert the boolean to integer type ? we can avoid the issue.
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #45564:
URL: https://github.com/apache/spark/pull/45564#issuecomment-2014820869
Thank you @stefanbuk-db @beliefer @dongjoon-hyun @milastdbx
Merged to master/3.5.2
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn closed pull request #45564: [SPARK-47440][SQL] Fix pushing unsupported syntax to MsSqlServer
URL: https://github.com/apache/spark/pull/45564
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on PR #45564:
URL: https://github.com/apache/spark/pull/45564#issuecomment-2004240228
> Thank you for making a PR, @stefanbuk-db . Is this MSSQLServer specific issue?
Yes, as far as I know this is a MSSQLServer specific issue, not an overall one.
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1535023323
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala:
##########
@@ -86,6 +87,31 @@ private object MsSqlServerDialect extends JdbcDialect {
case "STDDEV_SAMP" => "STDEV"
case _ => super.dialectFunctionName(funcName)
}
+
+ override def build(expr: Expression): String = {
+ def isChildBooleanExpression(child: Expression): Boolean = child match {
+ case _: Predicate => true
+ case _ => false
+ }
+
+ // MsSqlServer does not support boolean comparison using standard comparison operators
+ // We shouldn't propagate these queries to MsSqlServer
+ expr match {
+ case e: Predicate =>
+ e.name() match {
+ case "=" | "<>" | "<=>" | "<" | "<=" | ">" | ">=" =>
+ if (isChildBooleanExpression(e.children()(0))
+ || isChildBooleanExpression(e.children()(1))) {
+ super.visitUnexpectedExpr(expr)
+ } else {
+ super.build(expr)
+ }
+ case _ => super.build(expr)
+ }
+ case _ =>
+ super.build(expr)
+ }
+ }
Review Comment:
```suggestion
override def build(expr: Expression): String = {
// MsSqlServer does not support boolean comparison using standard comparison operators
// We shouldn't propagate these queries to MsSqlServer
expr match {
case e: Predicate => e.name() match {
case "=" | "<>" | "<=>" | "<" | "<=" | ">" | ">="
if e.children().exists(_.isInstanceOf[Predicate]) =>
super.visitUnexpectedExpr(expr)
case _ => super.build(expr)
}
case _ => super.build(expr)
}
}
```
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1535173845
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala:
##########
@@ -86,6 +87,20 @@ private object MsSqlServerDialect extends JdbcDialect {
case "STDDEV_SAMP" => "STDEV"
case _ => super.dialectFunctionName(funcName)
}
+
+ override def build(expr: Expression): String = {
+ // MsSqlServer does not support boolean comparison using standard comparison operators
+ // We shouldn't propagate these queries to MsSqlServer
+ expr match {
+ case e: Predicate => e.name() match {
+ case "=" | "<>" | "<=>" | "<" | "<=" | ">" | ">="
+ if e.children().exists(_.isInstanceOf[Predicate]) =>
+ super.visitUnexpectedExpr(expr)
Review Comment:
```suggestion
if e.children().exists(_.isInstanceOf[Predicate]) =>
super.visitUnexpectedExpr(expr)
```
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1535170069
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala:
##########
@@ -125,4 +125,20 @@ class MsSqlServerIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JD
},
errorClass = "_LEGACY_ERROR_TEMP_2271")
}
+
+ test("SPARK-47440: SQLServer does not support boolean expression in binary comparison") {
+ val df1 = sql(s"SELECT name FROM " +
Review Comment:
```suggestion
val df1 = sql("SELECT name FROM " +
```
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1529609383
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -2648,6 +2648,11 @@
"Cannot resolve window reference <windowName>."
]
},
+ "UNSUPPORTED_BINARY_COMPARISON_LIKE_OPERATOR" : {
Review Comment:
can we also have a test for the error case?
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1531664157
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala:
##########
@@ -125,4 +125,10 @@ class MsSqlServerIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JD
},
errorClass = "_LEGACY_ERROR_TEMP_2271")
}
+
+ test("SPARK-47440: SQLServer does not support LIKE in binary comparison") {
+ val df = sql(s"SELECT name FROM " +
+ s"$catalogName.employee WHERE ((name LIKE 'am%') = (name LIKE '%y'))")
Review Comment:
`WHERE ((name NOT LIKE 'am%') = (name NOT LIKE '%y'))` is not covered, right?
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1531724877
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala:
##########
@@ -125,4 +125,10 @@ class MsSqlServerIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JD
},
errorClass = "_LEGACY_ERROR_TEMP_2271")
}
+
+ test("SPARK-47440: SQLServer does not support LIKE in binary comparison") {
+ val df = sql(s"SELECT name FROM " +
+ s"$catalogName.employee WHERE ((name LIKE 'am%') = (name LIKE '%y'))")
Review Comment:
Yes, that is correct. LIKE is not the only thing that can cause problems with MsSqlServer syntax. In this case that would be operator NOT that also has a boolean return value, which is also a case for other logical operators. That all should be a part of a bigger 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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1535170244
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala:
##########
@@ -125,4 +125,20 @@ class MsSqlServerIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JD
},
errorClass = "_LEGACY_ERROR_TEMP_2271")
}
+
+ test("SPARK-47440: SQLServer does not support boolean expression in binary comparison") {
+ val df1 = sql(s"SELECT name FROM " +
+ s"$catalogName.employee WHERE ((name LIKE 'am%') = (name LIKE '%y'))")
+ assert(df1.collect().length == 4)
+
+ val df2 = sql(s"SELECT name FROM " +
+ s"$catalogName.employee " +
+ s"WHERE ((name NOT LIKE 'am%') = (name NOT LIKE '%y'))")
Review Comment:
```suggestion
"WHERE ((name NOT LIKE 'am%') = (name NOT LIKE '%y'))")
```
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala:
##########
@@ -125,4 +125,20 @@ class MsSqlServerIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JD
},
errorClass = "_LEGACY_ERROR_TEMP_2271")
}
+
+ test("SPARK-47440: SQLServer does not support boolean expression in binary comparison") {
+ val df1 = sql(s"SELECT name FROM " +
+ s"$catalogName.employee WHERE ((name LIKE 'am%') = (name LIKE '%y'))")
+ assert(df1.collect().length == 4)
+
+ val df2 = sql(s"SELECT name FROM " +
+ s"$catalogName.employee " +
+ s"WHERE ((name NOT LIKE 'am%') = (name NOT LIKE '%y'))")
Review Comment:
```suggestion
"WHERE ((name NOT LIKE 'am%') = (name NOT LIKE '%y'))")
```
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1531814484
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala:
##########
@@ -125,4 +125,10 @@ class MsSqlServerIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JD
},
errorClass = "_LEGACY_ERROR_TEMP_2271")
}
+
+ test("SPARK-47440: SQLServer does not support LIKE in binary comparison") {
+ val df = sql(s"SELECT name FROM " +
+ s"$catalogName.employee WHERE ((name LIKE 'am%') = (name LIKE '%y'))")
Review Comment:
Sure! What are next steps?
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1531828751
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala:
##########
@@ -86,6 +86,35 @@ private object MsSqlServerDialect extends JdbcDialect {
case "STDDEV_SAMP" => "STDEV"
case _ => super.dialectFunctionName(funcName)
}
+
+ override def build(expr: Expression): String = {
Review Comment:
Please don't override `build` which specify the specification of DS V2 framework.
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45564:
URL: https://github.com/apache/spark/pull/45564#issuecomment-2004210431
cc @yaooqinn , too
--
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-47440][SQL] Fix pushing unsupported syntax to MsSqlServer [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45564:
URL: https://github.com/apache/spark/pull/45564#discussion_r1531827394
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala:
##########
@@ -125,4 +125,10 @@ class MsSqlServerIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JD
},
errorClass = "_LEGACY_ERROR_TEMP_2271")
}
+
+ test("SPARK-47440: SQLServer does not support LIKE in binary comparison") {
+ val df = sql(s"SELECT name FROM " +
+ s"$catalogName.employee WHERE ((name LIKE 'am%') = (name LIKE '%y'))")
Review Comment:
let's add the above case to the test and make it work
--
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