You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "JkSelf (via GitHub)" <gi...@apache.org> on 2026/10/14 13:54:22 UTC
[PR] [VL] [TEST]Enable io cache [gluten]
JkSelf opened a new pull request, #13286:
URL: https://github.com/apache/gluten/pull/13286
<!--
Thank you for submitting a pull request! Here are some tips:
1. For first-time contributors, please read our contributing guide:
https://github.com/apache/gluten/blob/main/CONTRIBUTING.md
2. If necessary, create a GitHub issue for discussion beforehand to avoid duplicate work.
3. If the PR is specific to a single backend, include [VL] or [CH] in the PR title to indicate the
Velox or ClickHouse backend, respectively.
4. If the PR is not ready for review, please mark it as a draft.
-->
## What changes are proposed in this pull request?
<!--
Provide a clear and concise description of the changes introduced in this PR.
Ensure the PR description aligns with the code changes, especially after updates.
If applicable, include "Fixes #<GitHub_Issue_ID>" to automatically close the corresponding issue
when the PR is merged.
-->
## How was this patch tested?
<!--
Describe how the changes were tested, if applicable.
Include new tests to validate the functionality, if necessary.
For UI-related changes, attach screenshots to demonstrate the updates.
-->
## Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] [TEST]Enable io cache [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] [TEST]Enable io cache [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org