[SPARK-57988][SQL] Parenthesize IN and other non-comparison predicate operands of IS [NOT] NULL in V2ExpressionSQLBuilder#57080
Conversation
… operands of IS [NOT] NULL in V2ExpressionSQLBuilder Signed-off-by: joelrobin18 <joelrobin1818@gmail.com>
… and CONTAINS Signed-off-by: joelrobin18 <joelrobin1818@gmail.com>
cloud-fan
left a comment
There was a problem hiding this comment.
0 blocking, 0 non-blocking, 0 nits.
Clean generalization of SPARK-57243 — isNullOperandNeedsParens now wraps every operand family that renders non-self-delimiting (IN, AND/OR/NOT, LIKE-family, arithmetic) while leaving self-delimited operands (functions, CASE, CAST) unwrapped.
Verification
Traced the central question: can any operand reachable at visitIsNullOperand render non-self-delimiting yet escape the parens set? Enumerating every build() switch case, the set is the exact complement of the self-delimiting operators — everything excluded (functions, CASE_WHEN, TRIM, OVERLAY, EXTRACT, CAST) renders self-delimited, and the two non-self-delimiting names not in the set (IS_NULL/BOOLEAN_EXPRESSION) can't reach this path (IsNull(col) translates its child without isPredicate, and IsNull is non-nullable so nesting folds). The MsSqlServerDialect guard widening to isInstanceOf[Predicate] rejects exactly the boolean-typed operands T-SQL can't represent — closing a latent bug where IS NULL over IN/AND/LIKE previously fell through to super.build and pushed invalid SQL — while arithmetic (a GeneralScalarExpression, not a Predicate) is still pushed. Fix is complete across dialects (only MsSqlServer had such a guard).
… operands of IS [NOT] NULL in V2ExpressionSQLBuilder ### What changes were proposed in this pull request? `V2ExpressionSQLBuilder` currently parenthesizes the operand of `IS [NOT] NULL` only when it is a binary comparison (added in SPARK-57243 via `visitIsNullOperand`). Any other non-self-delimiting operand is rendered unwrapped, producing invalid or misparsed SQL, e.g. for an `IN` predicate: ```sql -- before (invalid: PostgreSQL reports "syntax error at or near NOT") "a" IN (1, 2) IS NOT NULL -- after ("a" IN (1, 2)) IS NOT NULL ``` This PR introduces an `isNullOperandNeedsParens()` hook that extends the parenthesization to every operator whose rendered SQL is not a self-delimiting primary: - binary comparisons (`=`, `<>`, `<=>`, `<`, `<=`, `>`, `>=`) -- as before (SPARK-57243) - `IN` - boolean connectives (`AND`, `OR`, `NOT`) -- without parens, `IS NULL` binds to only part of the operand (e.g. `NOT ("a" = 1) IS NULL` parses as `NOT (("a" = 1) IS NULL)`), silently changing results - LIKE-family (`STARTS_WITH`, `ENDS_WITH`, `CONTAINS`) -- the rendering ends with `ESCAPE '\'`, which collides with a trailing `IS NULL` - arithmetic (`+`, `-`, `*`, `/`, `%`, `&`, `|`, `^`, `~`) -- defensive, for dialects with nonstandard `IS NULL` precedence Function calls, `CASE ... END` and `CAST(...)` already render self-delimited and are intentionally left unwrapped, preserving SPARK-57243's rationale of not changing SQL that was already valid. `MsSqlServerDialect` is also updated: its guard that rejects `IS [NOT] NULL` over a binary comparison (T-SQL has no boolean value type, so a predicate cannot appear as an `IS NULL` operand at all -- parentheses cannot fix this) is widened to reject any predicate operand. `compileExpression` then returns `None` and Spark evaluates the filter locally instead of pushing SQL that is guaranteed to fail at runtime. Non-predicate operands such as arithmetic are still pushed down. ### Why are the changes needed? Catalyst's `BooleanSimplification` rewrites `col IN (...) OR col NOT IN (...)` into `IsNotNull(In(col, ...))`, so `IS [NOT] NULL` over an `IN` predicate reaches V2 pushdown even though the user never wrote it. The generated SQL is rejected by the external database (e.g. PostgreSQL: `syntax error at or near "NOT"`), failing the whole Spark query. ### Does this PR introduce _any_ user-facing change? Yes, but only as a bug fix. Previously, queries whose pushed filters contained `IS [NOT] NULL` over an `IN` (or other non-comparison) predicate failed with a syntax error from the external database (e.g. PostgreSQL: `syntax error at or near "NOT"`). With this fix the generated SQL is valid and such queries succeed. On SQL Server, where this construct cannot be expressed at all, the filter is no longer pushed down and Spark evaluates it locally, so those queries also succeed. No changes for queries that were already working. ### How was this patch tested? - New test in `JDBCSuite` covering the rendered SQL for `IN`, `AND`/`OR`/`NOT`, LIKE-family, arithmetic and function-call operands of `IS [NOT] NULL`, including the MsSqlServer no-pushdown and still-pushed-down cases. Verified it fails on master without the fix (`"a" IN (1, 2) IS NULL` vs `("a" IN (1, 2)) IS NULL`). - New end-to-end test in `JDBCV2Suite` verifying `(salary IN (10000, 12000)) IS [NOT] NULL` is pushed down and executes correctly on H2. - New test in `V2JDBCTest` (docker integration suites) verifying pushdown and results on real databases, reusing the `supportsIsNullOverPredicate` hook from SPARK-57243. - Ran `JDBCSuite`, `JDBCV2Suite`, `V2PredicateSuite` and `DataSourceV2StrategySuite` (252 tests, all pass). ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code This pull request and its description were written by Isaac. Closes #57080 from joelrobin18/SPARK-57988-isnull-predicate-operand-parens. Authored-by: Joel Robin P <joelrobin1818@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 4b3765b) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
What changes were proposed in this pull request?
V2ExpressionSQLBuildercurrently parenthesizes the operand ofIS [NOT] NULLonly when it is a binary comparison (added in SPARK-57243 viavisitIsNullOperand). Any other non-self-delimiting operand is rendered unwrapped, producing invalid or misparsed SQL, e.g. for anINpredicate:This PR introduces an
isNullOperandNeedsParens()hook that extends the parenthesization to every operator whose rendered SQL is not a self-delimiting primary:=,<>,<=>,<,<=,>,>=) -- as before (SPARK-57243)INAND,OR,NOT) -- without parens,IS NULLbinds to only part of the operand (e.g.NOT ("a" = 1) IS NULLparses asNOT (("a" = 1) IS NULL)), silently changing resultsSTARTS_WITH,ENDS_WITH,CONTAINS) -- the rendering ends withESCAPE '\', which collides with a trailingIS NULL+,-,*,/,%,&,|,^,~) -- defensive, for dialects with nonstandardIS NULLprecedenceFunction calls,
CASE ... ENDandCAST(...)already render self-delimited and are intentionally left unwrapped, preserving SPARK-57243's rationale of not changing SQL that was already valid.MsSqlServerDialectis also updated: its guard that rejectsIS [NOT] NULLover a binary comparison (T-SQL has no boolean value type, so a predicate cannot appear as anIS NULLoperand at all -- parentheses cannot fix this) is widened to reject any predicate operand.compileExpressionthen returnsNoneand Spark evaluates the filter locally instead of pushing SQL that is guaranteed to fail at runtime. Non-predicate operands such as arithmetic are still pushed down.Why are the changes needed?
Catalyst's
BooleanSimplificationrewritescol IN (...) OR col NOT IN (...)intoIsNotNull(In(col, ...)), soIS [NOT] NULLover anINpredicate reaches V2 pushdown even though the user never wrote it. The generated SQL is rejected by the external database (e.g. PostgreSQL:syntax error at or near "NOT"), failing the whole Spark query.Does this PR introduce any user-facing change?
Yes, but only as a bug fix. Previously, queries whose pushed filters contained
IS [NOT] NULLover anIN(or other non-comparison) predicate failed with a syntax error from the external database (e.g. PostgreSQL:syntax error at or near "NOT"). With this fix the generated SQL is valid and such queries succeed. On SQL Server, where this construct cannot be expressed at all, the filter is no longer pushed down and Spark evaluates it locally, so those queries also succeed. No changes for queries that were already working.How was this patch tested?
JDBCSuitecovering the rendered SQL forIN,AND/OR/NOT, LIKE-family, arithmetic and function-call operands ofIS [NOT] NULL, including the MsSqlServer no-pushdown and still-pushed-down cases. Verified it fails on master without the fix ("a" IN (1, 2) IS NULLvs("a" IN (1, 2)) IS NULL).JDBCV2Suiteverifying(salary IN (10000, 12000)) IS [NOT] NULLis pushed down and executes correctly on H2.V2JDBCTest(docker integration suites) verifying pushdown and results on real databases, reusing thesupportsIsNullOverPredicatehook from SPARK-57243.JDBCSuite,JDBCV2Suite,V2PredicateSuiteandDataSourceV2StrategySuite(252 tests, all pass).Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code
This pull request and its description were written by Isaac.