Copilot commented on code in PR #12400:
URL: https://github.com/apache/gluten/pull/12400#discussion_r3549074001
##########
ep/build-velox/src/get-velox.sh:
##########
@@ -164,6 +164,22 @@ function apply_compilation_fixes {
$SUDO_CMD cp ${CURRENT_DIR}/modify_arrow.patch
${VELOX_HOME}/CMake/resolve_dependency_modules/arrow/
git add
${VELOX_HOME}/CMake/resolve_dependency_modules/arrow/modify_arrow.patch # to
avoid the file from being deleted by git clean -dffx :/
+
+ # Wire file handle cache TTL config to SimpleLRUCache constructor.
+ if [ -f "${CURRENT_DIR}/file-handle-cache-ttl.patch" ]; then
+ pushd "${VELOX_HOME}" > /dev/null
+ if git apply --check "${CURRENT_DIR}/file-handle-cache-ttl.patch"
2>/dev/null; then
+ git apply "${CURRENT_DIR}/file-handle-cache-ttl.patch"
+ echo "Applied file-handle-cache-ttl.patch"
+ elif git apply --check -R "${CURRENT_DIR}/file-handle-cache-ttl.patch"
2>/dev/null; then
+ echo "file-handle-cache-ttl.patch already applied upstream, skipping"
+ else
+ popd > /dev/null
+ echo "ERROR: file-handle-cache-ttl.patch failed to apply and is not
present upstream" >&2
+ exit 1
+ fi
+ popd > /dev/null
+ fi
}
Review Comment:
This block only applies the TTL wiring patch if
`file-handle-cache-ttl.patch` exists. Since this PR relies on that patch to
ensure TTL eviction is actually wired for Velox versions that don’t have it
upstream, silently skipping when the patch file is missing can let builds
succeed without the advertised TTL behavior.
Consider treating a missing patch file as a hard error (or removing the
existence check and always running the apply/skip/fail logic), so the build
can’t accidentally ship without TTL wiring.
##########
gluten-substrait/src/main/scala/org/apache/gluten/config/GlutenConfig.scala:
##########
@@ -653,7 +653,7 @@ object GlutenConfig extends ConfigRegistry {
("spark.hadoop.dfs.client.log.severity", "INFO"),
("spark.sql.orc.compression.codec", "snappy"),
("spark.sql.decimalOperations.allowPrecisionLoss", "true"),
- ("spark.gluten.sql.columnar.backend.velox.fileHandleCacheEnabled",
"false"),
+ ("spark.gluten.sql.columnar.backend.velox.fileHandleCacheEnabled",
"true"),
("spark.gluten.velox.awsSdkLogLevel", "FATAL"),
Review Comment:
`getNativeBackendConf` hardcodes a default for
`velox.fileHandleCacheEnabled`, which helps keep the native backend config (and
any reuse keying based on it) stable whether the user explicitly sets the
default or not. The new file-handle cache tuning configs
(`numCacheFileHandles`, `fileHandleExpirationDurationMs`) are not included in
this default list, so two sessions with identical effective behavior (implicit
defaults vs explicitly set to default values) will produce different native
conf maps and may prevent backend/connector reuse.
Consider adding these two keys to the same default list with their
documented defaults (10000, 600000ms) to keep config propagation and reuse
behavior consistent.
--
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]
