ctagard opened a new pull request, #59403:
URL: https://github.com/apache/spark/pull/59403
### What changes were proposed in this pull request?
`PushPredicateThroughNonJoin` pushes a filter on grouping keys below an
`Aggregate`. This PR keeps a conjunct above the `Aggregate` when it can tell
apart values that the `Aggregate` merges by normalizing float/double grouping
keys.
Float/double grouping keys are normalized during physical planning
(`SparkStrategies`): `-0.0` becomes `0.0` and NaNs are canonicalized. The
normalized key keeps the original `ExprId`, so everything above the `Aggregate`
sees the normalized value, while a filter pushed below it sees the raw value.
The change, in the `Aggregate` case of `PushPredicateThroughNonJoin`:
- Collect the child attributes behind grouping expressions whose type needs
normalization (`NormalizeFloatingNumbers.needNormalize`).
- Push a conjunct only if `sensitiveToFloatNormalization(replaced,
normalizedKeys)` is false. The check runs on the conjunct after alias
expansion, which is the expression that actually runs below the `Aggregate`. So
an alias like `cast(x AS STRING) AS s` is caught.
- `sensitiveToFloatNormalization` walks the expression:
- A normalized key used as a direct float/double operand of a comparison
(`BinaryComparison`, `In`, `IsNull`, `IsNotNull`, `IsNaN`) is safe, because
these already treat `-0.0`/`0.0` and all NaNs as equal.
- Any other use of a key is treated as sensitive.
- Boolean connectives such as `OR` are sensitive if any child is.
Common filters such as `x = 1.0`, `x > 5` or `NOT isnan(x)` are still pushed
down. Filters such as `cast(x AS STRING) = '-0.0'` stay above the `Aggregate`.
### Why are the changes needed?
The rule assumes a grouping key has the same value above and below the
`Aggregate`, and normalization breaks that assumption for float/double keys. A
pushed filter can then return a row for which the predicate is false:
```python
spark.createDataFrame([(-0.0,)], "x double").createOrReplaceTempView("t")
spark.sql("SELECT x, cast(x AS STRING) = '-0.0' AS p FROM (SELECT DISTINCT x
FROM t)").collect()
# [Row(x=0.0, p=False)]
spark.sql("SELECT * FROM (SELECT DISTINCT x FROM t) WHERE cast(x AS STRING)
= '-0.0'").collect()
# expected: []
# before: [Row(x=0.0)]
```
The same happens with `GROUP BY`, `INTERSECT ALL`, `EXCEPT`/`subtract` and
`dropDuplicates`. Pushing such a filter can also split a group and change its
aggregates:
```sql
-- t.x = [-0.0, 0.0]
SELECT x, count(*) FROM t GROUP BY x HAVING cast(x AS STRING) = '0.0'
-- expected: (0.0, 2)
-- before: (0.0, 1)
```
This is the same assumption as in SPARK-60093 (`RemoveRedundantAggregates`),
in a different rule.
### Does this PR introduce _any_ user-facing change?
Yes, it fixes a correctness bug. Queries that filter an aggregate's
float/double grouping key with a predicate that can tell `-0.0` from `0.0`, or
one NaN from another, now return results consistent with the predicate. Such
predicates are no longer pushed below the aggregate. Other filters are pushed
down as before.
### How was this patch tested?
New tests:
- `FilterPushdownSuite`, three plan tests:
- the ticket's filter stays above the `Aggregate`;
- comparison filters on double/float keys are still pushed down, while an
`OR` with a sensitive branch stays up as a whole, and only the sensitive
conjunct of an `AND` stays up;
- aliases computed from a key (`cast(x AS STRING) AS s`, `length(cast(x AS
STRING)) AS k`) are checked after alias expansion, while a plain rename is
still pushed down.
- `DataFrameAggregateSuite`, one end-to-end test:
- `distinct`, `groupBy().count()`, `dropDuplicates`, `intersectAll`,
`except`, a float key, the ticket's SQL and an aliased key all return the
correct rows;
- the split-group `count` case returns `(0.0, 2)`;
- a pushed comparison still gives the right answer.
All new tests fail without the fix.
Existing suites:
- `build/sbt 'catalyst/testOnly org.apache.spark.sql.catalyst.optimizer.*'`
- `build/sbt 'sql/testOnly *DataFrameAggregateSuite'`
- `build/sbt 'sql/testOnly *PlanStabilitySuite
*PlanStabilityWithStatsSuite'`. No golden plan changes.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5.5)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
