[
https://issues.apache.org/jira/browse/SPARK-57988?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Joel Robin updated SPARK-57988:
-------------------------------
Description:
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}}
was:
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}}
> 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
> Priority: Minor
>
> 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]
