This is an automated email from the ASF dual-hosted git repository.
MaxGekk pushed a commit to branch branch-4.x
in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/branch-4.x by this push:
new 367efc230a08 [SPARK-57243][SQL] Wrap IsNull and IsNotNull in brackets
for V2 SQL builder
367efc230a08 is described below
commit 367efc230a084a8517200e0fbc21efd9c737be0f
Author: Milan Stefanovic <[email protected]>
AuthorDate: Mon Jun 15 15:18:14 2026 +0200
[SPARK-57243][SQL] Wrap IsNull and IsNotNull in brackets for V2 SQL builder
### What changes were proposed in this pull request?
In this PR i propose we update V2 SQL builder logic to be more safe and
wrap child of IsNull in paranethisis if child itself is binary comparison.
This way we are safer from perspective of operator precedence.
Some dialects such as snowflake for example treat `IS NULL` as higher
precedence than equality and hence can lead to correctness issues or failed
queries.
Alternative was to look at number of children of `IS NULL` child and wrap
for anything bigger than 1, but rationale why this appraoch was not choosen as
it could be that some dialects fail if scalar expression is wrapped in brackets
(e.g. `CONTAINS(a, 'b') IS NULL` would be translated to `(CONTAINS(a, 'b')) IS
NULL` and we want to avoid introducing this risk and pontentially a breaking
change.
### Why are the changes needed?
To increase robustness of dialects inheriting this SQL builder
### Does this PR introduce _any_ user-facing change?
Queries that previously were failing for (e.g. Snowflake and SQLServer)
will now succeed. All but SQL server will not push down IS NULL on binary
predicate properly or decide not to pushdown in case its not supported (for
SQLServer)
### How was this patch tested?
H2 Integration test.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code
Co-authored via Claude Code
Closes #56296 from milastdbx/effort/isnullfix.
Authored-by: Milan Stefanovic <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
(cherry picked from commit 118026dc9b9693613aab96ae6f4a3c126c6c3fdf)
Signed-off-by: Max Gekk <[email protected]>
---
.../sql/jdbc/v2/MsSqlServerIntegrationSuite.scala | 3 ++
.../org/apache/spark/sql/jdbc/v2/V2JDBCTest.scala | 36 ++++++++++++++++++++++
.../sql/connector/util/V2ExpressionSQLBuilder.java | 27 +++++++++++++---
.../apache/spark/sql/jdbc/MsSqlServerDialect.scala | 13 ++++++--
.../org/apache/spark/sql/jdbc/JDBCSuite.scala | 30 ++++++++++++++++++
.../org/apache/spark/sql/jdbc/JDBCV2Suite.scala | 12 ++++++++
6 files changed, 115 insertions(+), 6 deletions(-)
diff --git
a/connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala
b/connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala
index 2845f3a5f19f..85f23efcde44 100644
---
a/connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala
+++
b/connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MsSqlServerIntegrationSuite.scala
@@ -99,6 +99,9 @@ class MsSqlServerIntegrationSuite extends
DockerJDBCIntegrationV2Suite with V2JD
override val catalogName: String = "mssql"
override val db = new MsSQLServerDatabaseOnDocker
+ // MsSqlServer has no boolean type, so IS [NOT] NULL over a predicate is not
pushed down.
+ override protected def supportsIsNullOverPredicate: Boolean = false
+
override def sparkConf: SparkConf = super.sparkConf
.set("spark.sql.catalog.mssql", classOf[JDBCTableCatalog].getName)
.set("spark.sql.catalog.mssql.url", db.getJdbcUrl(dockerIp, externalPort))
diff --git
a/connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/V2JDBCTest.scala
b/connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/V2JDBCTest.scala
index 79366189c20d..8e04660686ed 100644
---
a/connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/V2JDBCTest.scala
+++
b/connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/V2JDBCTest.scala
@@ -701,6 +701,42 @@ private[v2] trait V2JDBCTest
assert(rows12(5).getString(0) ===
"special_character_underscorenot_present")
}
+ // Whether the dialect can push down IS [NOT] NULL over a predicate operand,
e.g.
+ // `(a = 10000) IS NULL`. MsSqlServer has no boolean type, so it cannot and
Spark evaluates it.
+ protected def supportsIsNullOverPredicate: Boolean = true
+
+ test("SPARK-57243: IS [NOT] NULL over a composite operand is pushed down") {
+ val tbl = s"$catalogName.composite_is_null"
+ withTable(tbl) {
+ // b is a string label so the result type is the same across dialects
(an INT column
+ // comes back as BigDecimal on some engines such as Oracle).
+ sql(s"CREATE TABLE $tbl (a INT, b VARCHAR(8))")
+ Seq[(Option[Int], String)]((Some(10000), "x"), (Some(20000), "y"),
(None, "z"))
+ .toDF("a", "b").write.insertInto(tbl)
+
+ // Binary comparison operand must be parenthesized as `(a = 10000) IS
[NOT] NULL`.
+ // The NULL row exercises the IS NULL branch: (NULL = 10000) is NULL, so
it matches
+ // IS NULL and not IS NOT NULL. Dialects without a boolean type (e.g.
SQL Server) cannot
+ // push this, but Spark evaluates it and returns the same rows.
+ val df1 = sql(s"SELECT b FROM $tbl WHERE (a = 10000) IS NOT NULL")
+ checkFilterPushed(df1, supportsIsNullOverPredicate)
+ assert(df1.collect().map(_.getString(0)).sorted === Array("x", "y"))
+
+ val df2 = sql(s"SELECT b FROM $tbl WHERE (a = 10000) IS NULL")
+ checkFilterPushed(df2, supportsIsNullOverPredicate)
+ assert(df2.collect().map(_.getString(0)) === Array("z"))
+
+ // An arithmetic operand is not a predicate, so it is pushed down by
every dialect.
+ val df3 = sql(s"SELECT b FROM $tbl WHERE (a + 10000) IS NOT NULL")
+ checkFilterPushed(df3)
+ assert(df3.collect().map(_.getString(0)).sorted === Array("x", "y"))
+
+ val df4 = sql(s"SELECT b FROM $tbl WHERE (a + 10000) IS NULL")
+ checkFilterPushed(df4)
+ assert(df4.collect().map(_.getString(0)) === Array("z"))
+ }
+ }
+
test("SPARK-57040: TABLESAMPLE with replacement is not pushed down") {
withTable(s"$catalogName.new_table") {
sql(s"CREATE TABLE $catalogName.new_table (col1 INT, col2 INT)")
diff --git
a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
index 903cb70a79eb..20ca3d2ac09e 100644
---
a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
+++
b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
@@ -115,19 +115,20 @@ public class V2ExpressionSQLBuilder {
return visitGetArrayItem(getArrayItem);
} else if (expr instanceof GeneralScalarExpression e) {
String name = e.name();
+ if (isBinaryComparisonOperator(name)) {
+ return visitBinaryComparison(name, e.children()[0], e.children()[1]);
+ }
return switch (name) {
case "IN" -> {
Expression[] expressions = e.children();
List<String> children = expressionsToStringList(expressions, 1,
expressions.length - 1);
yield visitIn(build(expressions[0]), children);
}
- case "IS_NULL" -> visitIsNull(build(e.children()[0]));
- case "IS_NOT_NULL" -> visitIsNotNull(build(e.children()[0]));
+ case "IS_NULL" -> visitIsNull(visitIsNullOperand(e.children()[0]));
+ case "IS_NOT_NULL" ->
visitIsNotNull(visitIsNullOperand(e.children()[0]));
case "STARTS_WITH" -> visitStartsWith(build(e.children()[0]),
build(e.children()[1]));
case "ENDS_WITH" -> visitEndsWith(build(e.children()[0]),
build(e.children()[1]));
case "CONTAINS" -> visitContains(build(e.children()[0]),
build(e.children()[1]));
- case "=", "<>", "<=>", "<", "<=", ">", ">=" ->
- visitBinaryComparison(name, e.children()[0], e.children()[1]);
case "BOOLEAN_EXPRESSION" ->
build(expr.children()[0]);
case "+", "*", "/", "%", "&", "|", "^" ->
@@ -207,6 +208,24 @@ public class V2ExpressionSQLBuilder {
return joinListToString(list, ", ", v + " IN (", ")");
}
+ // The binary comparison operators, kept in one place so build,
visitIsNullOperand and
+ // dialect overrides stay in sync.
+ protected boolean isBinaryComparisonOperator(String name) {
+ return switch (name) {
+ case "=", "<>", "<=>", "<", "<=", ">", ">=" -> true;
+ default -> false;
+ };
+ }
+
+ // Parenthesize a binary comparison operand so `col = 'x' IS NULL` renders as
+ // `(col = 'x') IS NULL`. Dialects such as Snowflake bind IS NULL tighter
than =.
+ protected String visitIsNullOperand(Expression operand) {
+ if (operand instanceof GeneralScalarExpression e &&
isBinaryComparisonOperator(e.name())) {
+ return "(" + build(operand) + ")";
+ }
+ return build(operand);
+ }
+
protected String visitIsNull(String v) {
return v + " IS NULL";
}
diff --git
a/sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala
b/sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala
index 295479583687..5cfec0cdcbd4 100644
--- a/sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala
+++ b/sql/core/src/main/scala/org/apache/spark/sql/jdbc/MsSqlServerDialect.scala
@@ -100,11 +100,20 @@ private case class MsSqlServerDialect() extends
JdbcDialect with NoLegacyJDBCErr
case _ => super.visitSQLFunction(funcName, inputs)
}
+ private def isBinaryComparison(e: Expression): Boolean = e match {
+ case p: Predicate => isBinaryComparisonOperator(p.name())
+ case _ => false
+ }
+
+ // MsSqlServer has no boolean type: a comparison expression cannot appear
as a value.
+ // IS NULL / IS NOT NULL over a comparison is rejected so Spark evaluates
it locally.
+ // Binary comparisons themselves are pushed down via inputToSQLNoBool,
which rewrites any
+ // boolean sub-expressions into bit-typed equivalents.
override def build(expr: Expression): String = {
- // MsSqlServer does not support boolean comparison using standard
comparison operators
- // We shouldn't propagate these queries to MsSqlServer
expr match {
case e: Predicate => e.name() match {
+ case "IS_NULL" | "IS_NOT_NULL" if
isBinaryComparison(e.children().head) =>
+ visitUnexpectedExpr(expr)
case "=" | "<>" | "<=>" | "<" | "<=" | ">" | ">=" =>
val Array(l, r) = e.children().map(inputToSQLNoBool)
visitBinaryComparison(e.name(), l, r)
diff --git a/sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala
b/sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala
index 01f0d2619c22..3a83a2549f92 100644
--- a/sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala
+++ b/sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala
@@ -937,6 +937,36 @@ class JDBCSuite extends SharedSparkSession {
assert(dialect.compileExpression(eqFalse).get === "\"a\" = (1 = 0)")
}
+ test("SPARK-57243: IS [NOT] NULL parenthesizes a predicate operand") {
+ val dialect = JdbcDialects.get("jdbc:")
+ val msSqlServer = JdbcDialects.get("jdbc:sqlserver://127.0.0.1/db")
+ val a = FieldReference("a")
+ val b = FieldReference("b")
+
+ // Every binary comparison operand is parenthesized for both IS NULL and
IS NOT NULL, and is
+ // not pushed down on MsSqlServer (no boolean type), so Spark evaluates it
locally.
+ for (op <- Seq("=", "<>", "<", "<=", ">", ">=");
+ (isNullOp, keyword) <- Seq("IS_NULL" -> "IS NULL", "IS_NOT_NULL" ->
"IS NOT NULL")) {
+ val cmp = new Predicate(op, Array[V2Expression](a, b))
+ val expr = new Predicate(isNullOp, Array[V2Expression](cmp))
+ assert(dialect.compileExpression(expr).get === s"""("a" $op "b")
$keyword""")
+ assert(msSqlServer.compileExpression(expr).isEmpty)
+ }
+
+ // `<=>` (null-safe equal) is also a comparison, so the operand is
parenthesized; it never
+ // returns NULL so IS NULL over it is always false, but the rendering is
still wrapped.
+ val nullSafeIsNull =
+ new Predicate("IS_NULL", Array[V2Expression](new Predicate("<=>",
Array[V2Expression](a, b))))
+ val nullSafeSql = dialect.compileExpression(nullSafeIsNull).get
+ assert(nullSafeSql.startsWith("(") && nullSafeSql.endsWith(") IS NULL"))
+ assert(msSqlServer.compileExpression(nullSafeIsNull).isEmpty)
+
+ // A bare column reference is not parenthesized and is pushed down even on
MsSqlServer.
+ val bareIsNull = new Predicate("IS_NULL", Array[V2Expression](a))
+ assert(dialect.compileExpression(bareIsNull).get === "\"a\" IS NULL")
+ assert(msSqlServer.compileExpression(bareIsNull).get === "\"a\" IS NULL")
+ }
+
test("SPARK-57332: escape backslash in LIKE pattern for
STARTS_WITH/ENDS_WITH/CONTAINS") {
// Default dialect: standard SQL string literals take backslash verbatim,
so the LIKE escape
// character `\` appears once in the ESCAPE clause and a literal backslash
in the value is
diff --git
a/sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala
b/sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala
index ba6945c7b1fd..742a895a2839 100644
--- a/sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala
+++ b/sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala
@@ -274,6 +274,18 @@ class JDBCV2Suite extends SharedSparkSession with
ExplainSuiteHelper {
}
}
+ test("SPARK-57243: IS [NOT] NULL over a composite operand is pushed down and
runs on H2") {
+ val df1 = sql("SELECT name FROM h2.test.employee WHERE (salary = 10000) IS
NOT NULL")
+ checkFiltersRemoved(df1)
+ checkPushedInfo(df1, "PushedFilters: [SALARY IS NOT NULL, (SALARY =
10000.00) IS NOT NULL]")
+ checkAnswer(df1, Seq(Row("amy"), Row("alex"), Row("cathy"), Row("david"),
Row("jen")))
+
+ val df2 = sql("SELECT name FROM h2.test.employee WHERE (salary = 10000) IS
NULL")
+ checkFiltersRemoved(df2)
+ checkPushedInfo(df2, "PushedFilters: [(SALARY = 10000.00) IS NULL]")
+ checkAnswer(df2, Seq.empty)
+ }
+
// TABLESAMPLE ({integer_expression | decimal_expression} PERCENT) and
// TABLESAMPLE (BUCKET integer_expression OUT OF integer_expression)
// are tested in JDBC dialect tests because TABLESAMPLE is not supported by
all the DBMS
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
