[SPARK-49695][SQL] Postgres fix xor push-down #48144

andrej-db · 2024-09-18T10:22:35Z

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.

urosstan-db · 2024-09-18T10:46:16Z

Can we add some tests?

MaxGekk

It would be nice if you add an integration test, for instance here *PostgresExpressionPushdownSuite*

andrej-db · 2024-09-18T12:31:01Z

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

urosstan-db · 2024-09-18T12:51:14Z

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

dongjoon-hyun

Thank you, @andrej-db . +1 for adding a test case definitely.

cc @huaxingao , too.

andrej-db · 2024-09-22T14:48:00Z

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

urosstan-db · 2024-09-23T09:41:55Z

connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/V2JDBCTest.scala

    testDatetime(s"$catalogAndNamespace.${caseConvert("datetime")}")
  }
+
+  test("xor operator push-down") {


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.

neat, I like it

urosstan-db

Approving latest iteration (with tests)

PostgresIntegrationSuite: add test

MaxGekk

Please, fix your test:

[info] - SPARK-49695: Postgres fix xor push-down *** FAILED *** (10 milliseconds)
[info]   org.apache.spark.sql.catalyst.ExtendedAnalysisException: [TABLE_OR_VIEW_NOT_FOUND] The table or view `bar` cannot be found. Verify the spelling and correctness of the schema and catalog.

milastdbx · 2024-10-24T09:24:51Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala

+
+  override def compileExpression(expr: Expression): Option[String] = {
+    val builder = new PostgresSQLBuilder()
+    try {


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

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

milastdbx

Consider refactoring this

...integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala

urosstan-db

LGTM, but please make test more assertive.

MaxGekk

@andrej-db Could you fix builds and retrigger intergration tests:

[error] /home/runner/work/spark/spark/connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala:237:25: not found: type Filter
[error]       plan.isInstanceOf[Filter]
[error]                         ^
[error] one error found

MaxGekk · 2024-12-04T14:39:19Z

...integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala

+    assert(rows.length == 1)
+    assert(rows(0).getInt(0) === 6)
+    assert(rows(0).getString(1) === "jen")


Could you use checkAnswer, please.

This file has mainly these asserts, wanted to make it in line with other tests

...integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala

…park/sql/jdbc/v2/PostgresIntegrationSuite.scala Co-authored-by: Maxim Gekk <[email protected]>

MaxGekk · 2024-12-04T19:30:34Z

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

MaxGekk · 2024-12-04T19:32:23Z

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

dongjoon-hyun · 2024-12-04T20:00:39Z

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.

LuciferYang · 2024-12-05T02:29:49Z

Thanks @dongjoon-hyun

### What changes were proposed in this pull request? Backport of the #48144 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. Closes #49071 from andrej-db/PGXORBackport. Lead-authored-by: Andrej Gobeljić <[email protected]> Co-authored-by: andrej-gobeljic_data <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

PostgresDialect: add PostgresSqlBuilder, add override for xor push-down

70cdc0e

github-actions bot added the SQL label Sep 18, 2024

PetarVasiljevic-DB approved these changes Sep 18, 2024

View reviewed changes

urosstan-db approved these changes Sep 18, 2024

View reviewed changes

MaxGekk requested changes Sep 18, 2024

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-49695] Postgres fix xor push-down~~ [SPARK-49695][SQL] Postgres fix xor push-down Sep 18, 2024

dongjoon-hyun reviewed Sep 18, 2024

View reviewed changes

RaleSapic approved these changes Sep 18, 2024

View reviewed changes

V2JDBCTest: add test

7e81576

andrej-db requested review from MaxGekk, dongjoon-hyun and urosstan-db September 22, 2024 14:48

urosstan-db approved these changes Sep 23, 2024

View reviewed changes

urosstan-db reviewed Sep 23, 2024

View reviewed changes

urosstan-db approved these changes Sep 23, 2024

View reviewed changes

V2JDBCTest: remove test

02cf3d3

PostgresIntegrationSuite: add test

MaxGekk requested changes Oct 5, 2024

View reviewed changes

andrej-db added 3 commits October 24, 2024 00:37

Update PostgresIntegrationSuite.scala

6df7f81

Update PostgresIntegrationSuite.scala

f2982ae

Merge branch 'master' into SPARK-49695-PostgresXOR

de4a5cb

andrej-db requested review from MaxGekk and urosstan-db October 23, 2024 22:41

Update PostgresIntegrationSuite.scala

f7aadd7

milastdbx reviewed Oct 24, 2024

View reviewed changes

milastdbx approved these changes Oct 24, 2024

View reviewed changes

urosstan-db reviewed Oct 24, 2024

View reviewed changes

...integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala Outdated Show resolved Hide resolved

urosstan-db approved these changes Oct 24, 2024

View reviewed changes

andrej-db added 3 commits October 24, 2024 11:54

Update PostgresIntegrationSuite.scala

6d18ec6

PostgresIntegrationSuite: override

7b95659

PostgresIntegrationSuite: import

485a48d

MaxGekk requested changes Nov 7, 2024

View reviewed changes

andrej-db added 7 commits November 25, 2024 14:44

Update PostgresIntegrationSuite.scala

309c130

Update PostgresIntegrationSuite.scala

dc78465

Merge branch 'apache:master' into SPARK-49695-PostgresXOR

ad10faf

Update PostgresIntegrationSuite.scala

823cf7c

Update PostgresIntegrationSuite.scala

9a9a6a2

fix

b843186

Update PostgresIntegrationSuite.scala

574b7fc

MaxGekk reviewed Dec 4, 2024

View reviewed changes

Update connector/docker-integration-tests/src/test/scala/org/apache/s…

cdf3724

…park/sql/jdbc/v2/PostgresIntegrationSuite.scala Co-authored-by: Maxim Gekk <[email protected]>

MaxGekk approved these changes Dec 4, 2024

View reviewed changes

MaxGekk closed this in 4248397 Dec 4, 2024

andrej-db mentioned this pull request Dec 5, 2024

[SPARK-49695][SQL][3.5] Postgres fix xor push-down #49071

Closed

[SPARK-49695][SQL] Postgres fix xor push-down #48144

[SPARK-49695][SQL] Postgres fix xor push-down #48144

Uh oh!

Conversation

andrej-db commented Sep 18, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

urosstan-db commented Sep 18, 2024

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

andrej-db commented Sep 18, 2024

Uh oh!

urosstan-db commented Sep 18, 2024

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

andrej-db commented Sep 22, 2024

Uh oh!

urosstan-db Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

andrej-db Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

urosstan-db left a comment

Choose a reason for hiding this comment

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

milastdbx Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

urosstan-db Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

andrej-db Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

milastdbx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

urosstan-db left a comment

Choose a reason for hiding this comment

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

MaxGekk Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

andrej-db Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MaxGekk commented Dec 4, 2024

Uh oh!

MaxGekk commented Dec 4, 2024

Uh oh!

dongjoon-hyun commented Dec 4, 2024

Uh oh!

LuciferYang commented Dec 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants