jackylee-ch commented on code in PR #12549:
URL: https://github.com/apache/gluten/pull/12549#discussion_r3949769967
##########
gluten-core/src/main/scala/org/apache/gluten/GlutenPlugin.scala:
##########
@@ -146,9 +146,9 @@ private object GlutenDriverPlugin extends Logging {
// Set off-heap size in bytes per task.
val taskSlots = SparkResourceUtil.getTaskSlots(conf)
- conf.set(GlutenCoreConfig.NUM_TASK_SLOTS_PER_EXECUTOR, taskSlots)
+ conf.set(GlutenCoreConfig.NUM_TASK_SLOTS_PER_EXECUTOR.key,
taskSlots.toString)
val offHeapPerTask = offHeapSize / taskSlots
- conf.set(GlutenCoreConfig.COLUMNAR_TASK_OFFHEAP_SIZE_IN_BYTES,
offHeapPerTask)
+ conf.set(GlutenCoreConfig.COLUMNAR_TASK_OFFHEAP_SIZE_IN_BYTES.key,
offHeapPerTask.toString)
Review Comment:
Checked. They are equivalent in effect, but the change is forced rather than
stylistic: both entries became `createOptional`, and `OptionalConfigEntry[T]
extends ConfigEntry[Option[T]]`, so `conf.set(entry, taskSlots)` no longer
typechecks — the value has to be wrapped.
Why the entries became optional: base declared them
`createWithDefaultString("-1")` and `("0")` ([b77fdef08
`GlutenCoreConfig.scala:137,171`](https://github.com/apache/incubator-gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/gluten-core/src/main/scala/org/apache/gluten/config/GlutenCoreConfig.scala#L137-L178)).
Those placeholders exist because the real value cannot be derived without a
`SparkConf` at hand, and native rejects them —
`GLUTEN_CHECK(numTaskSlotsPerExecutor >= 0)` in `VeloxBackend`, and `0` for the
per-task off-heap size collapses the partial-aggregation limit that native
otherwise sizes from `kMaxMemory`. Base got away with it because its
`nativeKeys` set only delivered a key that was present in the map, so the
placeholder was never delivered — the default existed purely as a JVM read
fallback. Under the declarative API the default *is* what gets delivered when
unset, so a placeholder default would start reaching native. `createOptional`
states the actual co
ntract: absent means "no `SparkConf` was at hand", which is the non-execution
case (e.g. `Dataset#summary`, which does not propagate confs).
The `-1` and `0` were never read on the JVM side either — there is no
`getConf` reader for `numTaskSlotsPerExecutor` anywhere in tree, only writers.
Happy to revert to `conf.set(entry.key, v.toString)` if you prefer a smaller
diff at these two lines, but going through the entry keeps the write side using
the same converter as the read side.
--
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]
