iemejia commented on code in PR #12400:
URL: https://github.com/apache/gluten/pull/12400#discussion_r3549133737
##########
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:
Fixed. Added `numCacheFileHandles` (default "10000") and
`fileHandleExpirationDurationMs` (default "600000") to the native conf defaults
list alongside `fileHandleCacheEnabled`, so the native conf map is consistent
regardless of whether users explicitly set these to their default values.
##########
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:
The existence check is intentional as a forward-compatibility pattern. The
patch file is checked into the repo at
`ep/build-velox/src/file-handle-cache-ttl.patch` — it can only be absent if
explicitly removed in a future commit (i.e., when Velox upstreams the TTL
wiring). Without the check, cleanup would require atomically removing both the
patch file and this script block. With the check, removing the patch alone is
sufficient — the block becomes a no-op. The apply/skip/fail logic inside the
block already prevents silent breakage when the file IS present.
--
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]
