iemejia commented on code in PR #12400:
URL: https://github.com/apache/gluten/pull/12400#discussion_r3546746931
##########
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:
These values are already validated on the Spark side via `.checkValue(_ > 0,
...)` for `numCacheFileHandles` and `.checkValue(_ >= 0, ...)` for
`fileHandleExpirationDurationMs` (see `VeloxConfig.scala:548-559`). SparkConf
validation runs at session creation time, so invalid values are rejected before
they ever reach the native layer.
That said, defense-in-depth is reasonable. However, looking at the existing
patterns in this file, there is no precedent for validating config values here
— the extractor simply reads and forwards them. Adding `GLUTEN_CHECK` guards
here would diverge from the rest of the code. If we want native validation, it
should probably live in Velox's own HiveConfig layer. I'd prefer to keep this
as-is and rely on the Spark-side checks, but happy to add a `GLUTEN_CHECK` if
maintainers feel strongly.
##########
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:
Good catch. Will reword to clarify that entries represent network
connections/sockets rather than per-file OS file descriptors, but they can
still count toward OS resource limits (ulimit -n).
--
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]
