sarutak commented on code in PR #59004:
URL: https://github.com/apache/spark/pull/59004#discussion_r4165617562
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/orc/OrcFilters.scala:
##########
@@ -280,8 +286,13 @@ private[sql] object OrcFilters extends OrcFiltersBase {
Some(builder.startNot()
.isNull(dataTypeMap(name).fieldName, getType(name)).end())
- case In(name, values) if dataTypeMap.contains(name) =>
- val castedValues = values.map(v => castLiteralValue(v,
dataTypeMap(name).fieldType))
+ // SPARK-59605: NULLs in an IN list are inert (`x IN (.., NULL)` matches
the same rows as IN
Review Comment:
Two small follow-ups on this comment block:
**1. The Parquet contrast isn't quite accurate.** I suggested this
comparison earlier, but my wording oversimplified `ParquetFilters`. Its
`sources.In` branch is three-way. For a small list (`<=
pushDownInFilterThreshold`) it goes through `values.distinct.flatMap(makeEq)`,
where a NULL element is kept as a per-value equality (`makeEq(.., null)`)
rather than rewritten to a separate `IS NULL`. Only the large-list
partial-push-down path OR-s a separate `makeEq(.., null)` with
`makeInPredicate(nonNulls)`. The large-list non-partial path pushes nothing. So
Parquet keeps the NULL as a set or equality member rather than always pushing
`IS NULL`.
**2. "over-inclusive but safe" depends on Spark re-applying the predicate.**
The `Not(In)` case is only safe because ORC pushdown is advisory and Spark
re-evaluates the full predicate after the scan. It is worth stating that
premise so a future reader doesn't reuse this in an ORC-only (for example
`stripSparkFilter`-style) context where it would return wrong rows.
Suggested rewrite of the comment:
```suggestion
// SPARK-59605: NULLs in an IN list are inert (`x IN (.., NULL)`
matches the same rows as IN
// on the non-null values) and ORC's `in` cannot take them, so they
are dropped. (Parquet
// instead keeps the NULL as a set or equality member.) An empty or
all-NULL list leaves
// nothing to push. Under Not(In), NOT(IN(non-nulls)) is
over-inclusive but safe because ORC
// pushdown is advisory and Spark re-applies the full predicate after
the scan.
```
Also, the same wording in the PR description ("Parquet keeps the NULL and
pushes it as `IS NULL`") has the same inaccuracy. Parquet keeps the NULL as an
equality or set member rather than always pushing a separate `IS NULL`, so it
would be good to adjust that sentence in the description as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
