[SPARK-50087] Robust handling of boolean expressions in CASE WHEN for MsSqlServer and future connectors #48621

andrej-db · 2024-10-23T14:17:06Z

What changes were proposed in this pull request?

This PR proposes to propagate the isPredicate info in V2ExpressionBuilder and wrap the children of CASE WHEN expression (only Predicates) with IIF(<>, 1, 0) for MsSqlServer. This is done to force returning an int instead of a boolean, as SqlServer cannot handle boolean expressions as a return type in CASE WHEN.

E.g.
CASE WHEN ... ELSE a = b END

Old behavior:
CASE WHEN ... ELSE a = b END = 1

New behavior:
Since in SqlServer a = 1 is appended to the CASE WHEN, THEN and ELSE blocks must return an int. Therefore the final expression becomes:
CASE WHEN ... ELSE IIF(a = b, 1, 0) END = 1

Why are the changes needed?

A user cannot work with an MsSqlServer data with CASE WHEN clauses or IF clauses if they wish to return a boolean value.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added tests to MsSqlServerIntegrationSuite

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

No

MaxGekk

@milastdbx Could you review this PR, please.

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

cloud-fan · 2024-10-23T15:23:49Z

...egration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala

    assert(df.collect().length == 2)
  }
+
+  test("SPARK-50087: SqlServer handle booleans in IF in SELECT test") {


does this provide extra test coverage than the following new test cases?

This is a specific edge case. If you use a single IF in a SELECT it won't be pushed down, but this is.

...egration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala

sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala

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

sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java

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

cloud-fan · 2024-10-24T02:37:34Z

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

+            val stringArray = e.children().grouped(2).flatMap { arr =>
+              arr.dropRight(1).map(inputToSQL) :+
+                (arr.last match {
+                  case p: Predicate if p.name() != "ALWAYS_TRUE" && p.name() != "ALWAYS_FALSE" =>


can we say more about what's special for ALWAYS_TRUE/ALWAYS_FALSE?

ALWAYS_TRUE gets translated to 1, and that is an int. If wrapped, it will fail with the same issue we are trying to fix.

ALWAYS_TRUE gets translated to 1

Where does it happen?

override def compileValue(value: Any): Any = value match { case booleanValue: Boolean => if (booleanValue) 1 else 0 case other => super.compileValue(other) }

In MsSqlServer

hmm, is it correct? What if the boolean is in a place that do need a boolean type, like a function that takes a boolean? This is unrelated to this PR though.

problem is that MsSql cannot ask for a boolean as it cannot handle booleans. Any other DB will know how to translate it so we are ok. Only way this poses a problem is if we say CASE WHEN TRUE THEN which makes no sense so we are good

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

sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java

sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala

sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java

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

MsSqlServerDialect: refactor V2ExpressionSQLBuilder: remove aux

milastdbx

Approved.
Just please add test for when booleans are not children of case when, if such test does not exist

milastdbx · 2024-10-28T10:48:15Z

...egration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala

+          |    UPPER(name) AS test_type,
+          |    name,
+          |    IF(
+          |      LOWER(name) = 'adfsaef' OR LOWER(name) = 'agadg',


nit:
consider changing test literals to something more meaningful.

its easier to check what answer you are expecting

milastdbx · 2024-10-28T10:48:51Z

...egration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala

+          |)
+          |SELECT * FROM dummy_new limit 1""".stripMargin
+    )
+    df.collect()


should we have some pushdown check ?

I'll add an assert with External query check

milastdbx · 2024-10-28T10:49:13Z

...egration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala

+          | ELSE (name = '1') END
+          |""".stripMargin
+    )
+    df.collect()


im not sure but do we have test case for when type is not boolean ?

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

….scala

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

…ialect.scala

andrej-db

Double-checked the code.

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

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

...egration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala

…park/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala

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

…ialect.scala

cloud-fan · 2024-11-21T12:56:05Z

thanks, merging to master!

beliefer · 2025-02-14T12:17:24Z

late LGTM.

core

7d78be3

github-actions bot added the SQL label Oct 23, 2024

MaxGekk reviewed Oct 23, 2024

View reviewed changes

revert MsSqlServerDialect build method

7839e1d

kostabiz-db reviewed Oct 23, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Oct 23, 2024

View reviewed changes

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

cloud-fan reviewed Oct 23, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Oct 23, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Oct 23, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala Outdated Show resolved Hide resolved

andrej-db added 3 commits October 23, 2024 20:19

remove visitCaseWhen, more intuitive predicate wrapping

0172c49

imports

7ae7397

fix

49d742e

cloud-fan reviewed Oct 24, 2024

View reviewed changes

sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java Outdated Show resolved Hide resolved

cloud-fan reviewed Oct 24, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Oct 24, 2024

View reviewed changes

MsSqlServerDialect: comment

c2001d9

cloud-fan reviewed Oct 24, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala Show resolved Hide resolved

cloud-fan reviewed Oct 24, 2024

View reviewed changes

sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java Outdated Show resolved Hide resolved

milastdbx reviewed Oct 25, 2024

View reviewed changes

andrej-db added 2 commits October 25, 2024 17:47

JdbcDialects: move aux here

8b1b2da

MsSqlServerDialect: refactor V2ExpressionSQLBuilder: remove aux

V2ExpressionBuilder: refactor

1f01b77

milastdbx approved these changes Oct 28, 2024

View reviewed changes

nit

7e36029

cloud-fan reviewed Nov 11, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Nov 11, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Nov 11, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Nov 11, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala Outdated Show resolved Hide resolved

andrej-db and others added 2 commits November 18, 2024 17:08

nit

ab79afe

Update JdbcDialects.scala

de38a8c

cloud-fan added 2 commits November 19, 2024 15:10

Update sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects…

21ec622

….scala

Update JdbcDialects.scala

468eb89

cloud-fan reviewed Nov 19, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala Outdated Show resolved Hide resolved

cloud-fan and others added 8 commits November 19, 2024 20:52

Update sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerD…

1bc39ee

…ialect.scala

Update JdbcDialects.scala

6a221c6

Update MsSqlServerDialect.scala

ef1bcc8

Update JdbcDialects.scala

5d94fa1

Update MsSqlServerDialect.scala

f94f8e5

Update MsSqlServerIntegrationSuite.scala

55084a3

Update MsSqlServerDialect.scala

7fb6b29

Update MsSqlServerIntegrationSuite.scala

ca0545a

andrej-db commented Nov 19, 2024

View reviewed changes

cloud-fan reviewed Nov 20, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Nov 20, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Nov 20, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Nov 20, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala Outdated Show resolved Hide resolved

Apply suggestions from code review

df2fe41

cloud-fan reviewed Nov 20, 2024

View reviewed changes

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

cloud-fan added 3 commits November 20, 2024 11:15

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

53a4220

…park/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala

Update MsSqlServerIntegrationSuite.scala

cbef9a5

Update JdbcDialects.scala

c990ec6

cloud-fan reviewed Nov 20, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala Outdated Show resolved Hide resolved

cloud-fan and others added 3 commits November 20, 2024 23:39

Update sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerD…

ee4d4fb

…ialect.scala

Update MsSqlServerIntegrationSuite.scala

6cff9f5

Merge branch 'apache:master' into SPARK-50087-CaseWhen

bca7ce8

cloud-fan closed this in 2e1c3dc Nov 21, 2024

cloud-fan mentioned this pull request Dec 6, 2024

[SPARK-47463][SQL] Use V2Predicate to wrap expression with return type of boolean #45589

Closed

[SPARK-50087] Robust handling of boolean expressions in CASE WHEN for MsSqlServer and future connectors #48621

[SPARK-50087] Robust handling of boolean expressions in CASE WHEN for MsSqlServer and future connectors #48621

Uh oh!

Conversation

andrej-db commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrej-db Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

milastdbx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrej-db left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cloud-fan commented Nov 21, 2024

Uh oh!

beliefer commented Feb 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

andrej-db commented Oct 23, 2024 •

edited

Loading

andrej-db Oct 24, 2024 •

edited

Loading