jackylee-ch commented on PR #12549:
URL: https://github.com/apache/gluten/pull/12549#issuecomment-5572604619
One more pass, this time asking only "does the new machinery itself have a
defect I can trigger" rather than "is a conf classified right". It found a
silent wrong-results bug that all four earlier passes missed, fixed in
`9c436b2bb`.
### `spark.sql.mapKeyDedupPolicy=exception` silently disabled the
duplicate-key error
Spark's own entry declares an upper-casing transform
(`spark-catalyst_2.12-3.5.5-sources`, `SQLConf.scala:3785-3794`) and native
compares the delivered value against the literal `"EXCEPTION"`
(`WholeStageResultIterator.cc:660-664`). Base delivered the raw string, so:
```sql
SET spark.sql.mapKeyDedupPolicy=exception; -- Spark accepts this and
normalizes it
SELECT map(1, 'a', 1, 'b'); -- vanilla Spark: raises. Gluten:
returns {1 -> 'b'}
```
because native fell into the `else` branch and set
`kThrowExceptionOnDuplicateMapKeys=false`. Pre-existing in base, and I
reproduced the same omission in the first version of this PR — which is the
annoying part, because the two sibling declarations in the same block already
guard against exactly this: `spark.sql.legacy.timeParserPolicy` upper-cases,
and `spark.io.compression.codec` lower-cases (added two commits ago for the
same reason). The contract in `ConfigBuilder.scala` says "a foreign conf
declares the same converter Spark / Hadoop declares"; this one did not.
### `spark.shuffle.file.buffer` lost its only validating reader — and native
was prefix-parsing
This PR deletes the JVM-side `conf.get(SHUFFLE_FILE_BUFFER_SIZE)` that used
to run Spark's converter *and* its `checkValue` (cap `MAX_ROUNDED_ARRAY_LENGTH
/ 1024` = 2097151 KiB). Nothing replaced it, and `std::stoll` stops at the
first character it cannot use without erroring. Combined with
`convertForNative` deliberately delivering a value the converter rejected as
the raw string, that gave:
| conf value | JVM delivers | native computed | intended |
|---|---|---|---|
| `1.5m` | `"1.5m"` (unparseable, passed through) | **1024 bytes** | 1572864
|
| `10x` | `"10x"` (invalid suffix) | **10240 bytes** | rejected by Spark |
| `4194304` | `"4194304"` | **4 GiB per open file**, from an arrow pool
Spark does not track | rejected by Spark |
The first two did not even log — the `catch` never ran. Fixed both ends: the
declaration now carries Spark's bound as well as its unit, and
`createPartitionWriter` requires `stoll` to consume the whole string and
enforces the same bound, falling back to `kDefaultShuffleFileBufferSize` with a
warning otherwise.
### Cleared, for the record
The same pass checked and could not instantiate: a class-init deadlock or
premature-`val` read in the conf objects (the reference graph is a DAG, and
`registerNativeConfs()` is the last statement of `GlutenConfig`'s body); an
exception escaping `selectRuntimeConf` / `selectBackendConf` (every converter
reachable from `convertForNative` throws only `IllegalArgumentException` or its
`NumberFormatException` subclass, both caught); a lost registration through the
`backendConfDelivered` latch (every conf object holding a `passToNative` conf
is force-initialized on the single-threaded plugin-init path first, and the two
registries not in any `confs()` override declare none); and default
round-tripping for all five terminal methods.
One low-severity item I documented rather than fixed: the SQLConf mirror for
`spark.gluten.sql.columnar.shuffle.codec` now carries a default of `lz4`, so
`spark.conf.get` and `SET -v` report `lz4` even when
`spark.io.compression.codec=zstd` makes the effective codec zstd. Only the
*reported* value is wrong — every Gluten read goes through the entry, which
resolves the fallback correctly. Base reported the key as unset. Expressing it
properly needs a Spark-side fallback entry, which `private[spark]` puts out of
reach; if you would rather it report nothing than report `lz4`, that is a
one-line change back to `createOptional` plus a `None` branch at the two call
sites.
@zhztheplayer @philo-he @weiting-chen @zhouyuan — that is the last of the
self-review; nothing outstanding from my side beyond the two design questions I
put to you above (`ConfigEntryForeignFallback`, and whether the session-scoped
`spark.io.compression.codec` inheritance should stay).
--
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]
