You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "andrej-db (via GitHub)" <gi...@apache.org> on 2024/09/18 10:22:38 UTC

[PR] [SPARK-49695] Postgres fix xor push-down [spark]

andrej-db opened a new pull request, #48144:
URL: https://github.com/apache/spark/pull/48144

   ### What changes were proposed in this pull request?
   This PR fixes the pushdown of ^ operator (XOR operator) for Postgres. Those two databases use this as exponent, rather then bitwise xor.
   
   Fix is consisted of overriding the SQLExpressionBuilder to replace the '^' character with '#'.
   ### Why are the changes needed?
   Result is incorrect.
   
   ### Does this PR introduce _any_ user-facing change?
   Yes. The user will now have a proper translation of the ^ operator.
   
   ### How was this patch tested?
   
   ### 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-49695][SQL] Postgres fix xor push-down [spark]

Posted by "urosstan-db (via GitHub)" <gi...@apache.org>.
urosstan-db commented on code in PR #48144:
URL: https://github.com/apache/spark/pull/48144#discussion_r1814637673


##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala:
##########
@@ -230,6 +230,14 @@ class PostgresIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCT
     }
   }
 
+  test("SPARK-49695: Postgres fix xor push-down") {
+    val df = spark.sql("select id, name from employee where id ^ 6 = 0")

Review Comment:
   ```suggestion
       val df = spark.sql("select id, name from employee where id ^ 6 = 0")
       assert(!df.queryExecution.optimizedPlan.exists { plan =>
         plan.isInstanceOf[Filter]
       })
   ```
   
   We should assert xor is actually pushed down here



-- 
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-49695][SQL] Postgres fix xor push-down [spark]

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48144:
URL: https://github.com/apache/spark/pull/48144#discussion_r1869662382


##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala:
##########
@@ -252,6 +253,17 @@ class PostgresIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCT
     }
   }
 
+  test("SPARK-49695: Postgres fix xor push-down") {
+    val df = spark.sql(s"select dept, name from $catalogName.employee where dept ^ 6 = 0")
+    val rows = df.collect()
+    assert(df.queryExecution.sparkPlan.collectFirst {
+      case f: FilterExec => f
+    }.isEmpty)
+    assert(rows.length == 1)
+    assert(rows(0).getInt(0) === 6)
+    assert(rows(0).getString(1) === "jen")

Review Comment:
   Could you use `checkAnswer`, please.



##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala:
##########
@@ -252,6 +253,17 @@ class PostgresIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCT
     }
   }
 
+  test("SPARK-49695: Postgres fix xor push-down") {
+    val df = spark.sql(s"select dept, name from $catalogName.employee where dept ^ 6 = 0")
+    val rows = df.collect()
+    assert(df.queryExecution.sparkPlan.collectFirst {
+      case f: FilterExec => f
+    }.isEmpty)

Review Comment:
   ```suggestion
       assert(!df.queryExecution.sparkPlan.exists(_.isInstanceOf[FilterExec]))
   ```



-- 
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-49695][SQL] Postgres fix xor push-down [spark]

Posted by "andrej-db (via GitHub)" <gi...@apache.org>.
andrej-db commented on code in PR #48144:
URL: https://github.com/apache/spark/pull/48144#discussion_r1869925135


##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala:
##########
@@ -252,6 +253,17 @@ class PostgresIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCT
     }
   }
 
+  test("SPARK-49695: Postgres fix xor push-down") {
+    val df = spark.sql(s"select dept, name from $catalogName.employee where dept ^ 6 = 0")
+    val rows = df.collect()
+    assert(df.queryExecution.sparkPlan.collectFirst {
+      case f: FilterExec => f
+    }.isEmpty)
+    assert(rows.length == 1)
+    assert(rows(0).getInt(0) === 6)
+    assert(rows(0).getString(1) === "jen")

Review Comment:
   This file has mainly these asserts, wanted to make it in line with other 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-49695][SQL] Postgres fix xor push-down [spark]

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #48144:
URL: https://github.com/apache/spark/pull/48144#issuecomment-2518391368

   @andrej-db Could you open separate PRs for 3.4 and 3.5 because your changes cause conflicts in the branches.


-- 
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-49695] Postgres fix xor push-down [spark]

Posted by "andrej-db (via GitHub)" <gi...@apache.org>.
andrej-db commented on PR #48144:
URL: https://github.com/apache/spark/pull/48144#issuecomment-2358342937

   I wanted to add tests, but didn't know where to put them... Don't know how oss tests postgre.


-- 
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-49695][SQL] Postgres fix xor push-down [spark]

Posted by "andrej-db (via GitHub)" <gi...@apache.org>.
andrej-db commented on code in PR #48144:
URL: https://github.com/apache/spark/pull/48144#discussion_r1771370209


##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/V2JDBCTest.scala:
##########
@@ -986,4 +986,13 @@ private[v2] trait V2JDBCTest extends SharedSparkSession with DockerIntegrationFu
   test("scan with filter push-down with date time functions") {
     testDatetime(s"$catalogAndNamespace.${caseConvert("datetime")}")
   }
+
+  test("xor operator push-down") {

Review Comment:
   neat, I like 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-49695][SQL] Postgres fix xor push-down [spark]

Posted by "urosstan-db (via GitHub)" <gi...@apache.org>.
urosstan-db commented on code in PR #48144:
URL: https://github.com/apache/spark/pull/48144#discussion_r1771077639


##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/V2JDBCTest.scala:
##########
@@ -986,4 +986,13 @@ private[v2] trait V2JDBCTest extends SharedSparkSession with DockerIntegrationFu
   test("scan with filter push-down with date time functions") {
     testDatetime(s"$catalogAndNamespace.${caseConvert("datetime")}")
   }
+
+  test("xor operator push-down") {

Review Comment:
   Maybe you can do explain formatted and check whether string contains `"id" # 3`, that will add a little bit of robustness.
   Another thing we can do is to make unit test, and just invoke compilation of XOR expression and check whether `col # constant` is result of compilation.



-- 
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-49695][SQL] Postgres fix xor push-down [spark]

Posted by "andrej-db (via GitHub)" <gi...@apache.org>.
andrej-db commented on code in PR #48144:
URL: https://github.com/apache/spark/pull/48144#discussion_r1814707831


##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala:
##########
@@ -404,4 +404,26 @@ private case class PostgresDialect()
       case _ =>
     }
   }
+
+  override def compileExpression(expr: Expression): Option[String] = {
+    val builder = new PostgresSQLBuilder()
+    try {

Review Comment:
   done



-- 
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-49695][SQL] Postgres fix xor push-down [spark]

Posted by "urosstan-db (via GitHub)" <gi...@apache.org>.
urosstan-db commented on code in PR #48144:
URL: https://github.com/apache/spark/pull/48144#discussion_r1814640394


##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala:
##########
@@ -404,4 +404,26 @@ private case class PostgresDialect()
       case _ =>
     }
   }
+
+  override def compileExpression(expr: Expression): Option[String] = {
+    val builder = new PostgresSQLBuilder()
+    try {

Review Comment:
   +1 here, let's override last possible method in chain of execution



-- 
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-49695][SQL] Postgres fix xor push-down [spark]

Posted by "andrej-db (via GitHub)" <gi...@apache.org>.
andrej-db commented on code in PR #48144:
URL: https://github.com/apache/spark/pull/48144#discussion_r1814691687


##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala:
##########
@@ -230,6 +230,14 @@ class PostgresIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCT
     }
   }
 
+  test("SPARK-49695: Postgres fix xor push-down") {
+    val df = spark.sql("select id, name from employee where id ^ 6 = 0")

Review Comment:
   done



-- 
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-49695][SQL] Postgres fix xor push-down [spark]

Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #48144:
URL: https://github.com/apache/spark/pull/48144#issuecomment-2518953729

   Thanks @dongjoon-hyun 


-- 
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-49695][SQL] Postgres fix xor push-down [spark]

Posted by "andrej-db (via GitHub)" <gi...@apache.org>.
andrej-db commented on PR #48144:
URL: https://github.com/apache/spark/pull/48144#issuecomment-2366823733

   Added the test, let me know if this is in order.


-- 
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-49695] Postgres fix xor push-down [spark]

Posted by "urosstan-db (via GitHub)" <gi...@apache.org>.
urosstan-db commented on PR #48144:
URL: https://github.com/apache/spark/pull/48144#issuecomment-2358136257

   Can we add some 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-49695][SQL] Postgres fix xor push-down [spark]

Posted by "milastdbx (via GitHub)" <gi...@apache.org>.
milastdbx commented on code in PR #48144:
URL: https://github.com/apache/spark/pull/48144#discussion_r1814621792


##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala:
##########
@@ -404,4 +404,26 @@ private case class PostgresDialect()
       case _ =>
     }
   }
+
+  override def compileExpression(expr: Expression): Option[String] = {
+    val builder = new PostgresSQLBuilder()
+    try {

Review Comment:
   perhaps its better to only override visitBinaryArithmetics?
   
   We have similar problem with some of the functions and we use `dialectFunctionName` to translate from Spark to local dialect



-- 
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-49695][SQL] Postgres fix xor push-down [spark]

Posted by "urosstan-db (via GitHub)" <gi...@apache.org>.
urosstan-db commented on code in PR #48144:
URL: https://github.com/apache/spark/pull/48144#discussion_r1814637673


##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala:
##########
@@ -230,6 +230,14 @@ class PostgresIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCT
     }
   }
 
+  test("SPARK-49695: Postgres fix xor push-down") {
+    val df = spark.sql("select id, name from employee where id ^ 6 = 0")

Review Comment:
   ```suggestion
       val df = spark.sql("select id, name from employee where id ^ 6 = 0")
       assert(!df.queryExecution.optimizedPlan.exists { plan =>
         plan.isInstanceOf[LogicalPlan]
       })
   ```
   
   We should assert xor is actually pushed down here



-- 
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-49695][SQL] Postgres fix xor push-down [spark]

Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #48144:
URL: https://github.com/apache/spark/pull/48144#issuecomment-2518444688

   To @MaxGekk and @andrej-db , Apache Spark 3.4 reached the End-Of-Support. 
   
   Only `branch-3.5` is open for backporting. 
   
   Also, cc @LuciferYang since he is the release manager for Apache Spark 3.5.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-49695][SQL] Postgres fix xor push-down [spark]

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #48144:
URL: https://github.com/apache/spark/pull/48144#issuecomment-2518388025

   +1, LGTM. Merging to master/3.5/3.4.
   Thank you, @andrej-db and @urosstan-db @RaleSapic @PetarVasiljevic-DB @milastdbx @dongjoon-hyun for review.


-- 
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-49695][SQL] Postgres fix xor push-down [spark]

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #48144: [SPARK-49695][SQL] Postgres fix xor push-down
URL: https://github.com/apache/spark/pull/48144


-- 
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-49695] Postgres fix xor push-down [spark]

Posted by "urosstan-db (via GitHub)" <gi...@apache.org>.
urosstan-db commented on PR #48144:
URL: https://github.com/apache/spark/pull/48144#issuecomment-2358386290

   > I wanted to add tests, but didn't know where to put them... Don't know how oss tests postgre.
   
   `V2JDBCTest` is base class, and there is derived class for Postgres `PostgresIntegrationSuite`


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