jackylee-ch commented on PR #12549:
URL: https://github.com/apache/gluten/pull/12549#issuecomment-5571503496
@zhztheplayer — I ran your own review question back over the whole diff
("are there any issues if we always pass X / does this part have to exist"),
and it found four things. Three are fixed in `369623589`; the fourth is a
design choice I would rather you make than make for you.
### Fixed: four keys were declared `passToNative()` without native reading
them
The rule the PR states is "`passToNative()` means native reads this key from
the conf map". These four did not meet it, which is the same rule the PR uses
to justify deleting `spark.gluten.velox.fs.s3a.retry.mode` — so keeping them
was internally inconsistent:
| Key | Where the value actually crosses the boundary |
|---|---|
| `spark.gluten.sql.columnar.shuffle.codec` | `createPartitionWriter` JNI
argument. Native declares `kShuffleCompressionCodec` (`GlutenConfig.h:95`) and
never reads it |
| `...shuffle.codecBackend` | same, `GlutenConfig.h:96` |
| `spark.gluten.shuffleWriter.bufferSize` | `nativeBufferSize` JNI argument.
The key string appears in **no** file under `cpp/` or `cpp-ch/` |
| `spark.sql.legacy.sizeOfNull` | nowhere — the value is baked as a
substrait literal in `ExpressionConverter`. Its declaration called itself
"documentation", which is not a good enough reason to put a key nobody reads on
both channels |
Declarations: 66 → 62.
### Fixed: `spark.io.compression.codec` had its parse rule declared twice,
differently
`registerConf(SPARK_IO_COMPRESSION_CODEC)` was `stringConf` (identity) while
`COLUMNAR_SHUFFLE_CODEC`, which falls back to the same key, lower-cases it.
Chasing which one is right turned up a real failure: native reads the key at
`WholeStageResultIterator.cc:610` and hands it to Velox's
`stringToCompressionKind`, which looks the value up in a **lower-case-keyed**
`unordered_map` and raises `VELOX_UNSUPPORTED` on a miss.
`spark.io.compression.codec=ZSTD` is valid Spark config — Spark lower-cases at
its own read site in `CompressionCodec.createCodec` — and it failed Gluten's
spill path. Pre-existing; it only became visible once the declaration had to
state a converter. Both paths now use `transform(_.toLowerCase)`.
### Fixed: two smaller ones
`NativeConfRegistry.isRuntimeKey` / `isBackendKey` were public with zero
production callers (tests only) — now `private[gluten]` with a "visible for
testing" note. And the design doc claimed `Component.confs()` is "the only hook
early enough for the backend channel", which is false for both in-tree
backends: `VeloxListenerApi.parseConf` and `CHListenerApi.initialize` each call
`ensureRegistered()` before their own native init. Corrected — `confs()` is now
described as the hook that does not require a component to know where its
native init happens, which is its actual value.
### For you to decide: should `ConfigEntryForeignFallback` exist?
This PR adds `TypedConfigBuilder.fallbackConf(fallbackKey, fallbackDefault)`
+ `ConfigEntryForeignFallback` alongside the pre-existing
`ConfigBuilder.fallbackConf(entry: ConfigEntry[T])` + `ConfigEntryFallback`.
One production user each. My stated reason for the new one — Spark's
`ConfigEntry` is `private[spark]` so it cannot appear in a signature — is
defeated by this PR's own `registerConf`, which mints a *Gluten* `ConfigEntry`
for a foreign key and already does so for this very key.
So it could collapse to:
```scala
val SPARK_IO_COMPRESSION_CODEC_ENTRY =
registerConf(SPARK_IO_COMPRESSION_CODEC)
.stringConf
.transform(_.toLowerCase(Locale.ROOT))
.passToNative()
.createWithDefaultString(GlutenConfigUtil.sparkIoCompressionCodecDefault)
val COLUMNAR_SHUFFLE_CODEC =
buildConf("spark.gluten.sql.columnar.shuffle.codec").stringConf...
.fallbackConf(SPARK_IO_COMPRESSION_CODEC_ENTRY)
```
That deletes `ConfigEntryForeignFallback` (~60 lines), one terminal method,
and one arm of `declaredDefault`. Costs: `ConfigEntryFallback` has to grow the
`readWithSource` / `fallbackKey` members `GlutenShuffleUtils` needs (moved, not
duplicated), and `spark.io.compression.codec` becomes always-delivered as `lz4`
instead of omitted when unset — behaviour-neutral, since native's own fallback
at `:610` is already `"lz4"`.
I did not do it unilaterally because it restructures the codec path late in
review and the benefit is one concept fewer rather than a fix. Say the word and
I will; otherwise I will leave it and open a follow-up issue.
--
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]
