dongjoon-hyun commented on PR #58580:
URL: https://github.com/apache/spark/pull/58580#issuecomment-5579953790
Reviewed this together with `UnwrapCastInBinaryComparison`,
`PushDownUtils.pushRuntimeFilters`, `InSubqueryExec` and the in-memory test
fixtures.
The change looks correct to me, and the approach is the right one. It fixes
a real bug where type coercion on the join keys silently disabled DSv2 runtime
filter pushdown entirely. Walking through the semantics:
- Wrapping the values in `InSet(col, values)` and running `unwrapCast` on it
is exactly the code the optimizer applies to a literal `InSet`, so there is no
room for the two to disagree.
- `simplifyIn` keeps only values that survive a round-trip cast, so the
dropped ones (out of range, rounded) could not have matched any row to begin
with. The pushed filter never becomes more selective than the original, which
is the property that matters here.
- `falseIfNotNull(col)` is NULL when `col` is null and FALSE otherwise; as a
filter both drop the row, so translating it to `AlwaysFalse` is right.
- `simplifyIn` returns `falseIfNotNull` when both `nullList` and
`canCastList` are empty, so the `InSet` branch always has a non-empty `hset`.
An empty `IN` can't reach the source.
- Nulls are preserved through `nullList`, matching the previous
`values.map(LiteralValue(...))` behavior.
- When the child is not a `Cast`, `unwrapCast` returns `None` on its first
pattern, so the existing path is preserved.
A few comments below, none of them blocking.
### 1. Matching on the shape of `falseIfNotNull` is fragile
```scala
case And(IsNull(_), Literal(null, BooleanType)) =>
Some(new AlwaysFalse())
```
This couples the caller to the *internal representation* of
`falseIfNotNull`. If `UnwrapCastInBinaryComparison` ever switches to, say,
`If(IsNull(e), Literal(null), FalseLiteral)`, this silently falls through to
`case _` and we go back to logging a warning and losing the pruning — compiling
and passing tests all the way. That is a cross-module coupling that is easy to
miss.
Two options: also open up `falseIfNotNull` and compare with
`semanticEquals`, or — probably better — expose a narrow helper instead of the
general `unwrapCast`, since the caller only ever passes an `InSet` and only
understands two of the many shapes `unwrapCast` can return. Something like:
```scala
private[sql] def unwrapCastInSet(inSet: InSet): Option[...]
```
Right now that contract exists only in the reviewer's head.
### 2. The new scaladoc doesn't state the contract the new caller depends on
> Returns None if the expression is not rewritten.
What the new call site actually depends on is that the result may be a
`falseIfNotNull` shape rather than an `InSet`. Since the method is being
widened to `private[sql]`, it would help to spell out the possible result
shapes — that also documents the coupling in (1).
### 3. The `InSet`/`Set` is built even when there is no cast
```scala
val inSet = InSet(in.child, values.toSet)
```
On the common path (no cast) this pays for the `Set` conversion and, as a
side effect, **deduplicates and reorders the values pushed to the source**. The
cost is negligible, but the behavior change is unrelated to the purpose of this
PR. Restricting the unwrap attempt to `in.child` being a `Cast` keeps the
existing path byte-for-byte and makes the intent clearer.
Also, `InSubqueryExec` already holds the same expression as a private `lazy
val inSet` (subquery.scala:126) — worth deciding explicitly whether to reuse it
or to accept the duplication.
### 4. The same gap remains in `translateScalarSubqueryFilterV2`
The sibling runtime path in `pushRuntimeFilters` has the identical problem:
```scala
case DynamicPruningExpression(e) =>
DataSourceV2Strategy.translateRuntimeFilterV2(e)
case o => DataSourceV2Strategy.translateScalarSubqueryFilterV2(o) // <--
here
```
`cast(a as bigint) = <scalar subquery>` cannot be unwrapped by the optimizer
either, because the other side is not a `Literal` until runtime, and
`translateFilterV2` does not strip the cast after literalization. Same root
cause, same symptom. Is that in scope here, or worth a follow-up JIRA?
### 5. Minor
- The blank line before `case other =>` was dropped; keeping the surrounding
style would be nice.
- For an `InSubqueryExec` whose `result` is still null, a non-pushable child
previously fell through to `case other` and only logged a warning; it now
throws `SparkException.internalError`. Unreachable at execution time and
arguably more consistent, but worth confirming it's intentional.
- The description says the end-to-end test covers V1 filters, V2 predicates
and Catalyst expressions. The Catalyst variant
(`SupportsRuntimeCatalystFiltering`) doesn't go through
`translateRuntimeFilterV2` at all and passes without this fix — still a fine
regression test on the other two, but the claim is a bit broader than the
coverage.
- The commit message says `Assisted-by:` while the PR description says
`Generated-by:`; worth aligning with the template.
--
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]
