Copilot commented on code in PR #12400:
URL: https://github.com/apache/gluten/pull/12400#discussion_r3546001745
##########
backends-velox/src/main/scala/org/apache/gluten/config/VeloxConfig.scala:
##########
@@ -530,10 +530,34 @@ object VeloxConfig extends ConfigRegistry {
val COLUMNAR_VELOX_FILE_HANDLE_CACHE_ENABLED =
buildStaticConf("spark.gluten.sql.columnar.backend.velox.fileHandleCacheEnabled")
.doc(
- "Disables caching if false. File handle cache should be disabled " +
- "if files are mutable, i.e. file content may change while file path
stays the same.")
+ "Enables caching of file handles to avoid repeated open/close overhead
on remote " +
+ "filesystems. Should be disabled if files are mutable, i.e. file
content may " +
+ "change while file path stays the same.")
.booleanConf
- .createWithDefault(false)
+ .createWithDefault(true)
+
+ val COLUMNAR_VELOX_NUM_CACHE_FILE_HANDLES =
+
buildStaticConf("spark.gluten.sql.columnar.backend.velox.numCacheFileHandles")
+ .doc(
+ "Maximum number of entries in the file handle cache. Each entry holds
an open " +
+ "file descriptor (local FS) or connection state (remote FS). Note
that on " +
+ "local filesystems, high values may approach the OS file descriptor
limit " +
+ "(ulimit -n). On remote object stores (S3, ABFS, GCS) entries are
HTTP " +
+ "connections, not OS file descriptors.")
Review Comment:
The docstring claims that on remote object stores (S3/ABFS/GCS) cache
entries are “HTTP connections, not OS file descriptors”. HTTP connections still
consume OS resources (typically sockets, which count toward file descriptor
limits), so this wording can mislead users into thinking `ulimit -n` is
irrelevant for remote storage. Suggest rewording to clarify that the resource
is usually a network connection/socket rather than a local file descriptor per
file, but it can still be subject to OS limits.
##########
docs/velox-configuration.md:
##########
@@ -55,6 +56,7 @@ nav_order: 16
| spark.gluten.sql.columnar.backend.velox.memInitCapacity
| 🔄 Dynamic | 8MB | The initial memory capacity to reserve
for a newly created Velox query memory pool.
|
|
spark.gluten.sql.columnar.backend.velox.memoryPoolCapacityTransferAcrossTasks
| 🔄 Dynamic | true | Whether to allow memory capacity transfer
between memory pools from different tasks.
|
| spark.gluten.sql.columnar.backend.velox.memoryUseHugePages
| 🔄 Dynamic | false | Use explicit huge pages for Velox
memory allocation.
|
+| spark.gluten.sql.columnar.backend.velox.numCacheFileHandles
| ⚓ Static | 10000 | Maximum number of entries in the file
handle cache. Each entry holds an open file descriptor (local FS) or connection
state (remote FS). Note that on local filesystems, high values may approach the
OS file descriptor limit (ulimit -n). On remote object stores (S3, ABFS, GCS)
entries are HTTP connections, not OS file descriptors.
|
Review Comment:
This row says remote object-store cache entries are “HTTP connections, not
OS file descriptors”. Network connections typically use sockets and still
consume OS file descriptors and other resources, so this phrasing can give a
false sense that `ulimit -n` is irrelevant. Please reword to clarify that
entries are usually network connections/sockets (not local file descriptors per
file), but can still be subject to OS limits.
##########
cpp/velox/utils/ConfigExtractor.cc:
##########
@@ -322,6 +322,10 @@ std::shared_ptr<facebook::velox::config::ConfigBase>
createHiveConnectorConfig(
hiveConfMap[facebook::velox::connector::hive::HiveConfig::kEnableFileHandleCache]
=
conf->get<bool>(kVeloxFileHandleCacheEnabled,
kVeloxFileHandleCacheEnabledDefault) ? "true" : "false";
+
hiveConfMap[facebook::velox::connector::hive::HiveConfig::kNumCacheFileHandles]
=
+ std::to_string(conf->get<int32_t>(kVeloxNumCacheFileHandles,
kVeloxNumCacheFileHandlesDefault));
+
hiveConfMap[facebook::velox::connector::hive::HiveConfig::kFileHandleExpirationDurationMs]
= std::to_string(
+ conf->get<int64_t>(kVeloxFileHandleExpirationDurationMs,
kVeloxFileHandleExpirationDurationMsDefault));
Review Comment:
`numCacheFileHandles` and `fileHandleExpirationDurationMs` are propagated to
Velox without any native-side validation. Because `getNativeBackendConf` passes
raw SparkConf strings through, users can set negative/zero values and they will
be forwarded into Velox, which may lead to undefined behavior or
hard-to-diagnose runtime failures. Consider validating these two values here
(<= 0 / < 0) and failing fast with a clear error.
--
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]
