Skip to content

Conversation

@andrej-db
Copy link
Contributor

@andrej-db andrej-db commented Dec 5, 2024

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.

@andrej-db andrej-db changed the title [SPARK-49695][SQL] Postgres fix xor push-down [SPARK-49695][SQL] Postgres fix xor push-down (3.5 backport) Dec 5, 2024
@github-actions github-actions bot added the SQL label Dec 5, 2024
Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrej-db Please, change PR's title, and replace (3.5 backport) by the tag [3.5]. Also add a link to the original PR to PR's description.

@andrej-db andrej-db changed the title [SPARK-49695][SQL] Postgres fix xor push-down (3.5 backport) [SPARK-49695][3.5][SQL] Postgres fix xor push-down Dec 5, 2024
@andrej-db
Copy link
Contributor Author

@andrej-db Please, change PR's title, and replace (3.5 backport) by the tag [3.5]. Also add a link to the original PR to PR's description.

Done. @MaxGekk

@MaxGekk MaxGekk changed the title [SPARK-49695][3.5][SQL] Postgres fix xor push-down [SPARK-49695][SQL][3.5] Postgres fix xor push-down Dec 5, 2024
@yaooqinn
Copy link
Member

yaooqinn commented Dec 5, 2024

Also cc @LuciferYang

@LuciferYang
Copy link
Contributor

@andrej-db
Copy link
Contributor Author

Maybe the tables don't have the same data as in master. Will check.

@LuciferYang
Copy link
Contributor

Thank you @andrej-db

}
}

override def compileExpression(expr: Expression): Option[String] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to only move this function from https://github.com/apache/spark/pull/48210/files to the branch-3.5? WDYT @MaxGekk and @yaooqinn

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That PR (#48210) should be backported as well. Maybe that one can be done first, and this one (#49071) will then be 1:1 with my previous (#48144).

We need the SqlBuilder and the compileExpression for this to work.

Copy link
Contributor

@LuciferYang LuciferYang Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#48210 is an improvement rather than a bug fix. If the current pr can solve the issue, this changes is fine to me. Of course, let's hear what others think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's backport only fix related code.

@LuciferYang
Copy link
Contributor

also cc @dongjoon-hyun

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM.

dongjoon-hyun pushed a commit that referenced this pull request Dec 7, 2024
### 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]>
@dongjoon-hyun
Copy link
Member

Merged to branch-3.5 for Apache Spark 3.5.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants