Copilot commented on code in PR #13286:
URL: https://github.com/apache/gluten/pull/13286#discussion_r4272157683
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxListenerApi.scala:
##########
@@ -82,7 +82,12 @@ class VeloxListenerApi extends ListenerApi with Logging {
s"${COLUMNAR_VELOX_CACHE_ENABLED.key} is set to get better
locality.")
}
- if (conf.get(COLUMNAR_VELOX_CACHE_ENABLED) && conf.get(LOAD_QUANTUM) > 8 *
1024 * 1024) {
+ // If load quantum is not set explicitly, parseConf caps it to the max
size supported by
+ // the Velox cache. Only reject an explicit value that exceeds the limit.
+ if (
+ conf.get(COLUMNAR_VELOX_CACHE_ENABLED) &&
conf.contains(LOAD_QUANTUM.key) &&
+ conf.get(LOAD_QUANTUM) > MAX_LOAD_QUANTUM_WITH_CACHE
+ ) {
Review Comment:
`MAX_LOAD_QUANTUM_WITH_CACHE` is defined in the companion object, but it’s
referenced unqualified inside the class. This typically won’t resolve and will
fail compilation. Qualify it as `VeloxListenerApi.MAX_LOAD_QUANTUM_WITH_CACHE`
(or move the constant into the class / a shared scope imported into the class).
##########
backends-velox/src/main/scala/org/apache/gluten/config/VeloxConfig.scala:
##########
@@ -154,10 +154,10 @@ object VeloxConfig extends ConfigRegistry {
val COLUMNAR_VELOX_CACHE_ENABLED =
buildStaticConf("spark.gluten.sql.columnar.backend.velox.cacheEnabled")
Review Comment:
The PR flips a major default (`cacheEnabled` from `false` to `true`), but
the PR description sections (“What changes…”, “How was tested…”, AI tooling
disclosure) are left blank/template. Please update the PR description to
explicitly call out the default behavior change, expected impact, and how it
was tested.
##########
backends-velox/src/main/scala/org/apache/gluten/config/VeloxConfig.scala:
##########
@@ -154,10 +154,10 @@ object VeloxConfig extends ConfigRegistry {
val COLUMNAR_VELOX_CACHE_ENABLED =
buildStaticConf("spark.gluten.sql.columnar.backend.velox.cacheEnabled")
.doc(
- "Enable Velox cache, default off. It's recommended to enable" +
+ "Enable Velox cache, default on. It's recommended to enable " +
"soft-affinity as well when enable velox cache.")
.booleanConf
- .createWithDefault(false)
+ .createWithDefault(true)
Review Comment:
The PR flips a major default (`cacheEnabled` from `false` to `true`), but
the PR description sections (“What changes…”, “How was tested…”, AI tooling
disclosure) are left blank/template. Please update the PR description to
explicitly call out the default behavior change, expected impact, and how it
was tested.
##########
backends-velox/src/main/scala/org/apache/gluten/config/VeloxConfig.scala:
##########
@@ -154,10 +154,10 @@ object VeloxConfig extends ConfigRegistry {
val COLUMNAR_VELOX_CACHE_ENABLED =
buildStaticConf("spark.gluten.sql.columnar.backend.velox.cacheEnabled")
.doc(
- "Enable Velox cache, default off. It's recommended to enable" +
+ "Enable Velox cache, default on. It's recommended to enable " +
"soft-affinity as well when enable velox cache.")
Review Comment:
The docstring contains the same grammar issue: “when enable velox cache”
should be “when enabling Velox cache” (and consistent capitalization helps
readability in generated docs).
##########
docs/get-started/VeloxLocalCache.md:
##########
@@ -10,7 +10,7 @@ local disk when reading from remote storage, and future read
requests for previo
local cache files. To enable local caching, the following configurations are
required:
```
-spark.gluten.sql.columnar.backend.velox.cacheEnabled // Enable Velox
cache. Default: false.
+spark.gluten.sql.columnar.backend.velox.cacheEnabled // Enable Velox
cache. Default: true.
Review Comment:
The text says the listed configs are “required” to enable local caching, but
`cacheEnabled` now defaults to `true`, so it’s no longer required for enabling.
Consider updating the wording and/or the snippet to reflect that `cacheEnabled`
is optional (only needed to disable or to be explicit), while the cache
path/size configs are what actually enable effective local caching.
--
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]
