jackylee-ch commented on code in PR #12549:
URL: https://github.com/apache/gluten/pull/12549#discussion_r3949767198
##########
gluten-core/src/main/scala/org/apache/gluten/config/GlutenCoreConfig.scala:
##########
@@ -115,34 +122,53 @@ object GlutenCoreConfig extends ConfigRegistry {
.createWithDefault(false)
val COLUMNAR_OVERHEAD_SIZE_IN_BYTES =
- buildConf("spark.gluten.memoryOverhead.size.in.bytes")
+ buildStaticConf("spark.gluten.memoryOverhead.size.in.bytes")
.internal()
+ .passToNative()
.doc(
- "Must provide default value since non-execution operations " +
- "(e.g. org.apache.spark.sql.Dataset#summary) doesn't propagate
configurations using " +
- "org.apache.spark.sql.execution.SQLExecution#withSQLConfPropagated")
+ "Memory overhead available to the Velox global memory manager, set by
VeloxListenerApi " +
+ "from the actual resource configuration. No default value: the value
cannot be derived " +
+ "without a SparkConf at hand, and native treats the key's absence as
'unbounded' - " +
+ "`VeloxBackend::init` falls back to `kMaxMemory`, so delivering a
placeholder 0 would " +
+ "instead build the global memory manager with zero capacity. Absent
for a " +
+ "non-execution operation (e.g.
org.apache.spark.sql.Dataset#summary), which does not " +
+ "propagate configurations via SQLExecution#withSQLConfPropagated.")
.bytesConf(ByteUnit.BYTE)
- .createWithDefaultString("0")
+ .createOptional
+ // No `passToNative`: native declares `kSparkOffHeapMemory` but reads it
nowhere. The ClickHouse
+ // backend does consume it, but JVM-side from the conf map through this
entry, so it does not need
+ // the native channel.
val COLUMNAR_OFFHEAP_SIZE_IN_BYTES =
buildConf("spark.gluten.memory.offHeap.size.in.bytes")
.internal()
+ .passToNative()
Review Comment:
Right, thanks — the doc string below it was updated when the conf started
being delivered but this lead-in comment was not. Removed in `33b47dbb2`; the
doc string already states why the key has to reach native (ClickHouse reads it
JVM-side out of the delivered backend conf map, in
`CHTransformerApi.postProcessNativeConfig`).
##########
gluten-substrait/src/main/scala/org/apache/gluten/config/GlutenConfig.scala:
##########
@@ -479,7 +478,9 @@ object GlutenConfig extends ConfigRegistry {
val SPARK_UNSAFE_SORTER_SPILL_READER_BUFFER_SIZE =
"spark.unsafe.sorter.spill.reader.buffer.size"
val SPARK_SHUFFLE_SPILL_DISK_WRITE_BUFFER_SIZE =
"spark.shuffle.spill.diskWriteBufferSize"
val SPARK_SHUFFLE_SPILL_COMPRESS = "spark.shuffle.spill.compress"
- val SPARK_SHUFFLE_SPILL_COMPRESS_DEFAULT: Boolean = true
+ // The codec `spark.gluten.sql.columnar.shuffle.codec` falls back to, and
its Spark default.
+ val SPARK_IO_COMPRESSION_CODEC = "spark.io.compression.codec"
+ val SPARK_IO_COMPRESSION_CODEC_DEFAULT = "lz4"
Review Comment:
Good catch — that was the last restated foreign default in the PR, so it
should not have survived. Fixed in `33b47dbb2`:
```scala
// GlutenConfigUtil, package org.apache.spark.sql.internal
def sparkIoCompressionCodecDefault: String =
org.apache.spark.internal.config.IO_COMPRESSION_CODEC.defaultValueString
```
The read has to sit in a `org.apache.spark.*` package because both
`ConfigEntry` and `IO_COMPRESSION_CODEC` are `private[spark]` — that is also
the reason `fallbackConf` takes the fallback as `(key, defaultString)` rather
than as the foreign `ConfigEntry` itself. `ShuffleCodecConfSuite` keeps
asserting the literal `"lz4"` independently, so the two cannot drift silently
in the other direction either.
--
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]
