jackylee-ch commented on code in PR #12549:
URL: https://github.com/apache/gluten/pull/12549#discussion_r3949759160
##########
gluten-core/src/main/scala/org/apache/gluten/config/GlutenCoreConfig.scala:
##########
@@ -39,7 +39,7 @@ class GlutenCoreConfig(conf: SQLConf) extends Logging {
def offHeapMemorySize: Long = getConf(COLUMNAR_OFFHEAP_SIZE_IN_BYTES)
- def taskOffHeapMemorySize: Long =
getConf(COLUMNAR_TASK_OFFHEAP_SIZE_IN_BYTES)
+ def taskOffHeapMemorySize: Long =
getConf(COLUMNAR_TASK_OFFHEAP_SIZE_IN_BYTES).getOrElse(0L)
Review Comment:
Checked this against base, and the accessor is not a new sentinel — it
preserves base exactly.
Base declared the entry `createWithDefaultString("0")` ([b77fdef08
`GlutenCoreConfig.scala:137-145`](https://github.com/apache/incubator-gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/gluten-core/src/main/scala/org/apache/gluten/config/GlutenCoreConfig.scala#L137-L145)),
so `getConf(entry)` already returned `0L` when the conf was unset.
`getOrElse(0L)` reproduces that value for value. The sole JVM-side consumer is
`CHConfig.scala:175`, `(taskOffHeapMemorySize * 0.9).toLong`, which is the same
arithmetic on the same number as before.
What did change is the *native* side, and that is the point of making the
entry `createOptional`: base put a `0` into the conf map for a non-execution
operation that does not propagate confs, and native reads absence as
`kMaxMemory` (`WholeStageResultIterator` sizing partial aggregation, and
ClickHouse leaving its own defaults). Delivering `0` there collapses those
limits; delivering nothing is what the entry now does.
So the optionality that matters is expressed where it is consumed. Changing
the JVM accessor to `Option[Long]` would change `CHConfig`'s behavior rather
than preserve it, which is why I left it.
--
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]
