[
https://issues.apache.org/jira/browse/SPARK-57988?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Wenchen Fan reassigned SPARK-57988:
-----------------------------------
Assignee: Joel Robin
> V2ExpressionSQLBuilder produces invalid SQL for IS [NOT] NULL over an IN (and
> other non-comparison predicate) operand
> ---------------------------------------------------------------------------------------------------------------------
>
> Key: SPARK-57988
> URL: https://issues.apache.org/jira/browse/SPARK-57988
> Project: Spark
> Issue Type: Bug
> Components: SQL
> Affects Versions: 4.1.2
> Reporter: Joel Robin
> Assignee: Joel Robin
> Priority: Minor
> Labels: pull-request-available
>
> h3. What happens
> {{V2ExpressionSQLBuilder}} (the DSv2 pushdown SQL generator used by JDBC
> dialects, connectors, and other V2 sources) serializes {{IS NULL}} / {{IS NOT
> NULL}} by appending the keyword to the operand's SQL. SPARK-57243 added
> {{visitIsNullOperand}} to parenthesize the operand, but it only does so when
> the operand is a binary comparison ({{{}={}}}, {{{}<>{}}}, {{{}<{}}}, …).
> When the operand is any other compound predicate, most importantly an {{IN}}
> predicate, it is emitted unparenthesized:
> {code:sql}
> "a" IN (1, 2) IS NOT NULL
> {code}
> This SQL is rejected by SQL parsers because the operand of {{IS [NOT] NULL}}
> must be a scalar expression. For example, PostgreSQL reports a syntax error
> near {{{}NOT{}}}.
> h3. Error
> Parsing the generated pushdown SQL fails, e.g.:
> {code}
> [PARSE_SYNTAX_ERROR] Syntax error at or near 'NOT'. SQLSTATE: 42601
> {code}
> (The exact offending token is {{IS}} or {{NOT}} depending on the surrounding
> predicate and the source dialect.) When the operand is pushed to a JDBC
> external engine the malformed SQL is rejected by that engine and surfaces as
> a {{JDBC_EXTERNAL_ENGINE_SYNTAX_ERROR}} instead.
> h3. How the predicate arises
> This is not a hand-written expression. An ordinary filter such as
> {code:sql}
> WHERE col IN ('a', 'b') OR col NOT IN ('a', 'b')
> {code}
> is simplified by Catalyst's {{BooleanSimplification}} (the {{p OR NOT p}}
> rule) to {{IsNotNull(In(col, 'a', 'b'))}} – an {{IN}} predicate wrapped by
> {{{}IsNotNull{}}}. When that scan is pushed to a V2 source, the builder emits
> the invalid string above and the query fails.
> h3. How to reproduce
> {{sources.IsNotNull(String)}} cannot nest an {{{}IN{}}}, so construct the V2
> predicate directly – this is exactly the shape {{V2ExpressionBuilder}}
> produces for the optimized {{{}IsNotNull(In(a, 1, 2)){}}}:
> {code:scala}
> val dialect = JdbcDialects.get("jdbc:")
> val a = FieldReference("a")
> val inPred = new Predicate("IN",
> Array[V2Expression](a, LiteralValue(1, IntegerType), LiteralValue(2,
> IntegerType)))
> Seq("IS_NULL" -> "IS NULL", "IS_NOT_NULL" -> "IS NOT NULL").foreach { case
> (op, kw) =>
> val expr = new Predicate(op, Array[V2Expression](inPred))
> println(dialect.compileExpression(expr))
> }
> {code}
> Actual (buggy) output:
> {code:java}
> Some("a" IN (1, 2) IS NULL)
> Some("a" IN (1, 2) IS NOT NULL)
> {code}
> Expected output:
> {code:java}
> Some(("a" IN (1, 2)) IS NULL)
> Some(("a" IN (1, 2)) IS NOT NULL)
> {code}
> h3. Root cause
> In {{V2ExpressionSQLBuilder.visitIsNullOperand}} the guard only parenthesizes
> binary-comparison operands:
> {code:java}
> protected String visitIsNullOperand(Expression operand) {
> if (operand instanceof GeneralScalarExpression e &&
> isBinaryComparisonOperator(e.name())) {
> return "(" + build(operand) + ")";
> }
> return build(operand);
> }
> {code}
> {{IN}} is not a binary comparison, so it is not wrapped. The same gap applies
> to the other operators whose rendered SQL is not a self-delimiting primary –
> the boolean connectives ({{{}AND{}}} / {{OR}} / {{{}NOT{}}}) and the
> LIKE-family – where the missing parentheses additionally cause a precedence
> mis-association (e.g. {{a AND b IS NULL}} parses as {{{}a AND (b IS
> NULL){}}}), not just a parse error. On lines that predate SPARK-57243 the
> operand is not parenthesized at all, so even binary comparisons are affected
> there.
> h3. Fix
> Extend {{visitIsNullOperand}} to parenthesize the operand for the operators
> that, like binary comparisons, render as non-self-delimiting SQL –
> {{{}IN{}}}, the boolean connectives, the LIKE-family, and arithmetic.
> Function calls, {{{}CASE{}}}, and {{CAST}} already render self-delimited
> ({{{}f(...){}}}, {{{}CASE ... END{}}}, {{{}CAST(...){}}}) and are
> intentionally left unwrapped so already-valid generated SQL does not change:
> {code:java}
> protected String visitIsNullOperand(Expression operand) {
> if (operand instanceof GeneralScalarExpression e &&
> isNullOperandNeedsParens(e.name())) {
> return "(" + build(operand) + ")";
> }
> return build(operand);
> }
> // Operators whose rendered SQL is not a self-delimiting primary and
> therefore must be
> // parenthesized before a trailing IS [NOT] NULL: binary comparisons
> (SPARK-57243) plus IN,
> // the boolean connectives, LIKE-family, and arithmetic.
> protected boolean isNullOperandNeedsParens(String name) {
> return isBinaryComparisonOperator(name) || switch (name) {
> case "IN", "NOT", "AND", "OR", "STARTS_WITH", "ENDS_WITH", "CONTAINS",
> "+", "-", "*", "/", "%", "&", "|", "^", "~" -> true;
> default -> false;
> };
> }
> {code}
> Adding parentheses is semantics-preserving, so the change cannot alter query
> results – only the generated SQL string, and only for operands that
> previously produced invalid (or precedence-wrong) SQL.
> h3. Impact
> Any DataSource V2 implementation that relies on {{V2ExpressionSQLBuilder}} to
> serialize pushed filters may generate invalid SQL that pushes an {{IsNull}} /
> {{IsNotNull}} over a non-comparison predicate through
> {{V2ExpressionSQLBuilder}} receives invalid SQL and the query fails. The
> {{col IN (...) OR col NOT IN (...)}} -> {{IsNotNull(In)}} simplification is a
> common way to hit it. Pre-existing and long-standing.
> h3. Testing
> New regression test in {{org.apache.spark.sql.jdbc.JDBCSuite}} asserting the
> parenthesized output for both {{IS NULL}} and {{IS NOT NULL}} over an {{IN}}
> operand. Validated locally, all pass, no regressions:
> * {{JDBCSuite}} (includes the new test)
> * {{DataSourceV2StrategySuite}}
> * {{JDBCV2Suite}}
> * {{MsSqlServerDialectSuite}}
> * {{PostgresDialectSuite}}
> * {{DataSourceV2DataFrameSuite}}
> * {{V2ExpressionRoundTripTrackerSuite}}
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
