Joel Robin created SPARK-57988:
----------------------------------
Summary: 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
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. 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]
