[SPARK-50792][SQL] Format binary data as a binary literal in JDBC. #49452

sunxiaoguang · 2025-01-11T08:27:18Z

What changes were proposed in this pull request?

Format binary data as a binary literal in JDBC.

Why are the changes needed?

The binary data is not handled to format it as binary literal in JDBC connectors
These are the steps to reproduce.

CREATE TABLE test_binary_literal(b BINARY);
INSERT INTO test_binary_literal VALUES(x'010203');
SELECT * FROM test_binary_literal WHERE b=x'010203';

Does this PR introduce any user-facing change?

'No'

How was this patch tested?

Added new integration tests

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

'No'

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

beliefer · 2025-01-11T09:52:37Z

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

+      val select = s"SELECT * FROM $catalogName.$tableName WHERE binary_col = $binary"
+      assert(spark.sql(select).collect().length === 1, s"Binary literal test failed: $select")
+    }
+  }


Please add the similar test case into JDBCV2Suite.

Please prepare data at tablePreparation.

tablePreparation

Sorry, I'm not quite familiar with the test infrastructure. In case I make mistakes, let me confirm this question.

To mixin the tablePreparation and dataPreparation from trait defined in V2JDBCTest.scala, we need to update all the integration tests and call the these functions defined in trait.

And duplicate the extra call to multiple integration tests is OK, am I right?

Hm, Just realized I have to use Spark SQL to create table and use the types defined in Spark SQL. If I prepare table and data in tablePreparation and dataPreparation, that will have to be database specific. The code will definitely have to be duplicated for connectors of all the databases.

Yes. If we update the basic class JdbcDialect, we should test all the built-in integration tests.
tablePreparation used to customize the DDL, I'm afraid Spark SQL can covers all the built-in integration tests. But you could do your best effort, let's see the result and make the decision.

Wait, just realized each dialect embeds a builder which can override the implementation. Let me have a try.

Oracle support is ready for review, PTAL. Thanks.

You can override visitBinaryComparison in OracleSQLBuilder.

There are couple threads discussing this topic. Let me copy the comment in case it's missed.

We can only rewrite comparison when one of the arguments is BLOB. For other cases, we have to use existing implementation. But unfortunately, the signature of visitBinaryComparison is accepting everything in string which loss the type information to understand if one of the arguments is binary type.

protected String visitBinaryComparison(String name, String l, String r) { if (name.equals("<=>")) { return "((" + l + " IS NOT NULL AND " + r + " IS NOT NULL AND " + l + " = " + r + ") " + "OR (" + l + " IS NULL AND " + r + " IS NULL))"; } return l + " " + name + " " + r; }

All tests passed, but downloading report failed. We can rerun the whole test again to clear all the checks, but it takes quite some time to finish.
https://github.com/sunxiaoguang/spark/actions/runs/12744731853

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

Depends on apache#49452 and apache#49453 Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

beliefer · 2025-01-12T12:30:34Z

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

+      // Unfornately, Oracle can only compare two BLOBs with a special function dbms_lob.compare
+      // The V2ExpressionSQLBuilder can not support rewriting the '=' operator.
+      // We don't test binary data on Oracle and do not support binary data on Oracle.
+      if (!this.isInstanceOf[OracleIntegrationSuite]) {


The test case skips the Oracle, but the users still possible use this case.

Yes, that requires a significant change in how V2ExpressionSQLBuilder works. The binary literal was not working before this PR anyway. We can make it work at least on other databases this time. And propose another design changes to V2ExpressionSQLBuilder to make it flexible enable to support cases like this.

As Oracle is supported, the test is now enabled for Oracle.

That's not a good idea if the bug still exists when using oracle dialect. Is there a way to fix the bug?

Yes, that requires a significant change in how V2ExpressionSQLBuilder works. The binary literal was not working before this PR anyway. We can make it work at least on other databases this time. And propose another design changes to V2ExpressionSQLBuilder to make it flexible enable to support cases like this.

Why we need change V2ExpressionSQLBuilder? Please describe the detail.

Sorry for the confusion; I didn't check the inheritance structure initially. After realizing that each dialect embeds a builder inherited from V2ExpressionSQLBuilder, I made all the changes in OracleDialect. PTAL

BTW: I just changed test to verify the returned result in addition. It's going to run for a while.

All tests passed, but downloading report failed. We can rerun the whole test again to clear all the checks, but it takes quite some time to finish.
https://github.com/sunxiaoguang/spark/actions/runs/12744731853

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

beliefer · 2025-01-13T11:40:22Z

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

+      }
+    }
+
+    override def build(expr: Expression): String = expr match {


You just need override visitBinaryComparison

We can only rewrite comparison when one of the arguments is BLOB. For other cases, we have to use existing implementation. But unfortunately, the signature of visitBinaryComparison is accepting everything in string which loss the type information to understand if one of the arguments is binary type.

protected String visitBinaryComparison(String name, String l, String r) { if (name.equals("<=>")) { return "((" + l + " IS NOT NULL AND " + r + " IS NOT NULL AND " + l + " = " + r + ") " + "OR (" + l + " IS NULL AND " + r + " IS NULL))"; } return l + " " + name + " " + r; }

All tests passed, but downloading report failed. We can rerun the whole test again to clear all the checks, but it takes quite some time to finish.
https://github.com/sunxiaoguang/spark/actions/runs/12744731853

LGTM. The code override build looks good to me. But we may need to add the protected method protected String visitBinaryComparison(String name, Expression l, Expression r) in V2ExpressionSQLBuilder.
What's your opinion? @cloud-fan

Cool, if we agree changing the signature of visitBinaryComparison in this PR, I can make the change all together to make the code easier to maintain.

Please wait a moment. We should change the signature in another PR if we agree on.

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

cloud-fan · 2025-01-15T07:35:45Z

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

+      testBinaryLiteral("<>", lessThanBinary, 1)
+      testBinaryLiteral("<=>", binary, 1)
+      testBinaryLiteral("<=>", lessThanBinary, 0)
+      testBinaryLiteral("<=>", greaterThanBinary, 0)


We will run these tests with all builtin dialects?

Right, the V2JDBCTest trait mixed in existing integrations and run against all the dialects. With this test, we figured out that Microsoft SQL Server, Oracle and PostgreSQL require extra work to support Binary Literal.

cloud-fan · 2025-01-15T07:38:12Z

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

+      val op = if (operator == "<=>") "=" else operator
+      val compare = s"DBMS_LOB.COMPARE($l, $r) $op 0"
+      if (operator == "<=>") {
+        s"(($l IS NOT NULL AND $r IS NOT NULL AND $compare) OR ($l IS NULL AND $r IS NULL))"


This is dirty but I don't have better ideas...

Yes, it's dirty. Fortunately, we only found Oracle requires a special function to compare blob values.

Even if we change signature of visitBinaryComparison to make the override better, we still cannot get rid of these dirty rewrites of <=> and DBMS_LOB.COMPARE.

cloud-fan · 2025-01-15T07:42:47Z

sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala

@@ -3097,4 +3097,19 @@ class JDBCV2Suite extends QueryTest with SharedSparkSession with ExplainSuiteHel
    assert(rows.contains(Row(null)))
    assert(rows.contains(Row("a a a")))
  }
+
+  test("SPARK-50792 Format binary data as a binary literal in JDBC.") {


I think for dialect-specific fixes, it's sufficient to put the tests V2JDBCTest.scala. This test suite is for testing the shared code paths for all dialects.

Right, this PR is somewhat complicated, it has changes in shared code paths to support Binary Literal. And also has changes in MsSqlServerDialect, OracleDialect and PostgresDialect. If this test is not necessary, we can remove it.

…park/sql/jdbc/v2/V2JDBCTest.scala Co-authored-by: Wenchen Fan <cloud0fan@gmail.com>

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

milastdbx

Question: Prior to these changes is there any scenario where some data sources might have returned something, although incorrect ? Is there a chance that someone has adapted to this "wrong" behaviour ?

Should we protect this change behind a flag ?

My understanding is that this never produced any kind of results that customers could have adapted to - hence flag is not required. Is this your understanding as well ?

Also. please lets check that pushdown is happening, to make sure we are testing what we want

milastdbx · 2025-01-15T11:34:29Z

sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala

+      sql(s"CREATE TABLE $tableName (binary_col BINARY)")
+      sql(s"INSERT INTO $tableName VALUES ($binary)")
+
+      val select = s"SELECT * FROM $tableName WHERE binary_col = $binary"


lets verify that this is actually pushed down - e.g. verify that there is no FilterExec present in plan

Verification added, PTAL. Thanks

milastdbx · 2025-01-15T11:48:51Z

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

+      sql(s"INSERT INTO $tableName VALUES ($binary)")
+
+      def testBinaryLiteral(operator: String, literal: String, expected: Int): Unit = {
+        val sql = s"SELECT * FROM $tableName WHERE binary_col $operator $literal"


lets verify that pushdown is present

checkFilterPushed

Check added, PTAL. Thanks

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

milastdbx · 2025-01-15T14:37:44Z

Can you provide your take on this:

My understanding is that this never produced any kind of results that customers could have adapted to - hence flag is not required. Is this your understanding as well ?

sunxiaoguang · 2025-01-15T14:48:50Z

Question: Prior to these changes is there any scenario where some data sources might have returned something, although incorrect ? Is there a chance that someone has adapted to this "wrong" behaviour ?

Should we protect this change behind a flag ?

My understanding is that this never produced any kind of results that customers could have adapted to - hence flag is not required. Is this your understanding as well ?

Also. please lets check that pushdown is happening, to make sure we are testing what we want

Yes your understanding is correct. Before this PR, queries using binary literal would generates SQL with invalid binary literal and causes SQL to fail.

cloud-fan · 2025-01-16T07:53:47Z

thanks, merging to master/4.0!

### What changes were proposed in this pull request? Format binary data as a binary literal in JDBC. ### Why are the changes needed? The binary data is not handled to format it as binary literal in JDBC connectors These are the steps to reproduce. 1. CREATE TABLE test_binary_literal(b BINARY); 2. INSERT INTO test_binary_literal VALUES(x'010203'); 3. SELECT * FROM test_binary_literal WHERE b=x'010203'; <img width="1180" alt="image" src="https://github.com/user-attachments/assets/92800c55-5400-46b0-b3f1-d95b85d89cb5" /> ### Does this PR introduce _any_ user-facing change? 'No' ### How was this patch tested? Added new integration tests ### Was this patch authored or co-authored using generative AI tooling? 'No' Closes #49452 from sunxiaoguang/fix_binary_literal_format_in_jdbc. Lead-authored-by: Xiaoguang Sun <sunxiaoguang@gmail.com> Co-authored-by: Xiaoguang Sun <sunxiaoguang@users.noreply.github.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 813591f) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

Format binary data as a binary literal in JDBC.

ca63d5a

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

github-actions bot added the SQL label Jan 11, 2025

beliefer changed the title ~~[SPARK-50792] Format binary data as a binary literal in JDBC.~~ [SPARK-50792][SQL] Format binary data as a binary literal in JDBC. Jan 11, 2025

beliefer reviewed Jan 11, 2025

View reviewed changes

Support binary literal for Oracle

7fbb786

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

sunxiaoguang mentioned this pull request Jan 11, 2025

[SPARK-50704][SQL] Support more pushdown functions for MySQL connector #49335

Open

sunxiaoguang added 5 commits January 11, 2025 18:36

Support binary literal for Oracle and add a test

ce6bc59

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

Format binary literal for PostgreSQL

13bc3ab

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

Use Spark SQL in JDBCV2Suite

2d86ab9

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

Refactor test code

d3bb055

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

Fix Oracle and DB2 binary literal

09022ab

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

sunxiaoguang added a commit to sunxiaoguang/spark that referenced this pull request Jan 11, 2025

Refactor PR to move fixes on two other PRs

a9575ca

Depends on apache#49452 and apache#49453 Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

sunxiaoguang added a commit to sunxiaoguang/spark that referenced this pull request Jan 11, 2025

Refactor PR to move fixes on two other PRs

604a1e3

Depends on apache#49452 and apache#49453 Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

Do not support binary literal on Oracle

8ddfcc2

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

sunxiaoguang force-pushed the fix_binary_literal_format_in_jdbc branch from f729330 to 8ddfcc2 Compare January 11, 2025 17:16

beliefer reviewed Jan 12, 2025

View reviewed changes

sunxiaoguang added 3 commits January 12, 2025 23:52

Implement binary literal for OracleDialect

84ea312

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

Support all binary comparison for BLOBs on Oracle

86a1cda

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

Verify returned result

b8c9708

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

beliefer reviewed Jan 13, 2025

View reviewed changes

beliefer reviewed Jan 14, 2025

View reviewed changes

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

cloud-fan reviewed Jan 15, 2025

View reviewed changes

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

cloud-fan reviewed Jan 15, 2025

View reviewed changes

sunxiaoguang and others added 2 commits January 15, 2025 17:25

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

6014e8e

…park/sql/jdbc/v2/V2JDBCTest.scala Co-authored-by: Wenchen Fan <cloud0fan@gmail.com>

Update test name

39d3dbb

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

milastdbx reviewed Jan 15, 2025

View reviewed changes

Check if filter is pushed in test

53d786e

Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>

milastdbx approved these changes Jan 15, 2025

View reviewed changes

cloud-fan closed this in 813591f Jan 16, 2025

sunxiaoguang deleted the fix_binary_literal_format_in_jdbc branch January 16, 2025 08:20

HyukjinKwon mentioned this pull request Jan 20, 2025

[SPARK-50792][SQL][FOLLOWUP] Improve the push down information for binary #49555

Open

[SPARK-50792][SQL] Format binary data as a binary literal in JDBC. #49452

[SPARK-50792][SQL] Format binary data as a binary literal in JDBC. #49452

Conversation

sunxiaoguang commented Jan 11, 2025 • edited Loading

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunxiaoguang Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

sunxiaoguang Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

beliefer Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunxiaoguang Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunxiaoguang Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunxiaoguang Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunxiaoguang Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunxiaoguang Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunxiaoguang Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunxiaoguang Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunxiaoguang Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

milastdbx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milastdbx commented Jan 15, 2025

sunxiaoguang commented Jan 15, 2025

cloud-fan commented Jan 16, 2025

sunxiaoguang commented Jan 11, 2025 •

edited

Loading

sunxiaoguang Jan 11, 2025 •

edited

Loading

sunxiaoguang Jan 11, 2025 •

edited

Loading

beliefer Jan 11, 2025 •

edited

Loading

sunxiaoguang Jan 13, 2025 •

edited

Loading

sunxiaoguang Jan 12, 2025 •

edited

Loading

sunxiaoguang Jan 13, 2025 •

edited

Loading

sunxiaoguang Jan 13, 2025 •

edited

Loading

sunxiaoguang Jan 14, 2025 •

edited

Loading

sunxiaoguang Jan 15, 2025 •

edited

Loading

sunxiaoguang Jan 15, 2025 •

edited

Loading

sunxiaoguang Jan 15, 2025 •

edited

Loading