Copilot commented on code in PR #13286:
URL: https://github.com/apache/gluten/pull/13286#discussion_r4271900045
##########
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:
Changing the default of
`spark.gluten.sql.columnar.backend.velox.cacheEnabled` from `false` to `true`
is a behavior-breaking change for existing deployments (it can introduce new
memory/disk IO usage and potentially change performance characteristics). If
this is intended only for a particular environment (e.g., tests/benchmarks),
consider keeping the default `false` and enabling it via test-specific configs,
or gate the default behind a separate feature flag. If the new default is
intended for all users, it should be explicitly called out as a breaking change
(e.g., in release notes / upgrade notes) and the docs should clearly describe
the operational impact and required sizing/path configs.
##########
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.
spark.gluten.sql.columnar.backend.velox.memCacheSize // In-memory cache
size. Default: 128MB.
spark.gluten.sql.columnar.backend.velox.ssdCachePath // Folder to store
cache files, preferably on SSD. Default: "/tmp".
spark.gluten.sql.columnar.backend.velox.ssdCacheSize // SSD cache size.
Memory-only caching is used when set to 0. Default: 128MB.
Review Comment:
This section introduces these as 'required' configs to enable local caching,
but with `cacheEnabled` now defaulting to `true`, it’s no longer strictly
required to set it. Consider revising the surrounding text to either (a)
describe `cacheEnabled` as optional (override to disable), or (b) restructure
the list into 'cache sizing/path configs' vs 'optional enable/disable toggle'
to avoid implying `cacheEnabled` must be set.
##########
cpp/velox/config/VeloxConfig.h:
##########
@@ -136,6 +136,7 @@ const std::string kHiveConnectorId = "test-hive";
const std::string kIcebergConnectorId = "test-iceberg";
const std::string kVeloxCacheEnabled =
"spark.gluten.sql.columnar.backend.velox.cacheEnabled";
+const bool kVeloxCacheEnabledDefault = true;
Review Comment:
In headers, prefer `inline constexpr bool kVeloxCacheEnabledDefault = true;`
(or align with whatever pattern is used for other `*Default` constants) to make
intent explicit and avoid any linkage/ODR ambiguity if this constant’s address
is ever used. This also better signals compile-time constness.
--
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]
