dongjoon-hyun opened a new pull request, #60067:
URL: https://github.com/apache/spark/pull/60067
### What changes were proposed in this pull request?
This PR fixes a correctness issue where `ConstantFolding` folds
`size(map(...))` and `cardinality(map(...))` to the number of key-value pairs,
although building the map removes duplicate keys. With
`spark.sql.mapKeyDedupPolicy=LAST_WIN`, the query returns a wrong size.
- `ConstantFolding`: remove the `Size(CreateMap)` case. When all children of
the map are foldable, the generic `foldable` case still folds the expression,
and it builds the map with `ArrayBasedMapBuilder` to get the real size. The
`Size(CreateArray)` case is unchanged.
- `CreateMap`: remove `NoThrow`. Building a map throws `NULL_MAP_KEY` on a
NULL key, and `DUPLICATED_MAP_KEY` on a duplicate key under the default
`EXCEPTION` policy. Because of `NoThrow`, `size(array(map(k, 1)))` was folded
to `1` without building the map.
- Add a migration guide entry.
Folding the size only when the keys are distinct non-null literals was also
considered. That check would have to repeat how `ArrayBasedMapBuilder` compares
keys: `0.0` and `-0.0` are one key (and so are NaN values), strings with a
non-binary collation are compared by their collation, and keys are often
wrapped in a `Cast` after type coercion. It would only help a map with literal
keys and non-literal values, such as `size(map('k1', a, 'k2', b))`. The plan
that [SPARK-33544](https://issues.apache.org/jira/browse/SPARK-33544)
optimized, the `size(...) > 0` filter inferred for `explode(map(c1, c2))`, has
not been generated since
[SPARK-37392](https://issues.apache.org/jira/browse/SPARK-37392), because
`InferFiltersFromGenerate` now infers filters only from attribute inputs. So
this PR removes the fold instead.
### Why are the changes needed?
This is a correctness issue. The fold has been there since SPARK-33544
([v3.1.1](https://github.com/apache/spark/releases/tag/v3.1.1), 2021-02-22).
```sql
SET spark.sql.mapKeyDedupPolicy=LAST_WIN;
-- Issue 1: It should return 1, but returns 2.
SELECT size(map(1, 'a', 1, 'b'));
-- Issue 2: It should return 1, but returns 2.
CREATE TABLE t(a STRING, b STRING) USING parquet;
INSERT INTO t VALUES ('x', 'x');
SELECT cardinality(map(a, 1, b, 2)) FROM t;
```
Under the default `EXCEPTION` policy, and for NULL keys under any policy,
the fold also hides the error that building the map raises, so the result
depends on whether `ConstantFolding` runs.
```sql
SET spark.sql.mapKeyDedupPolicy=EXCEPTION;
-- Issue 3: It should fail with DUPLICATED_MAP_KEY, but returns 2.
SELECT size(map(1, 'a', 1, 'b'));
-- Issue 4: It should fail with NULL_MAP_KEY, but returns 1.
CREATE TABLE n(k STRING) USING parquet;
INSERT INTO n VALUES (NULL);
SELECT size(map(k, 1)) FROM n;
-- Issue 5: It should fail with NULL_MAP_KEY, but returns 1.
SELECT size(array(map(k, 1))) FROM n;
```
With
`spark.sql.optimizer.excludedRules=org.apache.spark.sql.catalyst.optimizer.ConstantFolding`,
all of these queries already return the expected results.
### Does this PR introduce _any_ user-facing change?
Yes. `size` and `cardinality` of a map built by `map(...)` return the number
of entries of the built map, as when `ConstantFolding` is excluded.
- With `LAST_WIN`, a map with duplicate keys returns the deduplicated size,
for example `1` instead of `2` for `size(map(1, 'a', 1, 'b'))`.
- With `EXCEPTION`, a map with duplicate keys fails with
`DUPLICATED_MAP_KEY` instead of returning the number of pairs.
- A map with a NULL key fails with `NULL_MAP_KEY` instead of returning the
number of pairs.
`size(map(...))` with a non-foldable key or value is no longer folded during
optimization, and is evaluated at runtime instead. The migration guide is
updated.
### How was this patch tested?
Pass the CIs with a newly added test case in `ConstantFoldingSuite`. It
fails without this PR: the first part folds the size to `2` instead of `1`, and
the second part folds `size(array(map(a, 1)))` to `1`.
```
$ build/sbt 'catalyst/testOnly org.apache.spark.sql.catalyst.optimizer.*'
$ build/sbt 'sql/testOnly *DataFrameFunctionsSuite'
$ build/sbt 'sql/testOnly *SQLQueryTestSuite -- -z map.sql -z array.sql -z
mapconcat.sql -z mapZipWith.sql -z distinct-map-aggregates.sql'
```
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 5.5
--
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]
