iemejia commented on code in PR #12400:
URL: https://github.com/apache/gluten/pull/12400#discussion_r3616781243
##########
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:
This is safe on the Gluten path. Gluten forwards confs to native via
`GlutenConfigUtil.getConfString`, which calls the config entry's
`readFrom(...)`; for a `timeConf(MILLISECONDS)` entry that parses the value
(whether the user wrote `600000` or `10min`) into a `Long` of milliseconds and
then `.toString`s it. So the native side always receives a plain numeric
string, and `conf->get<int64_t>` never sees a unit suffix. Keeping `timeConf`
is also the project convention for duration configs and was requested by
@jackylee-ch in an earlier thread.
##########
backends-velox/src/main/scala/org/apache/gluten/config/VeloxConfig.scala:
##########
@@ -534,10 +535,35 @@ 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
represent " +
+ "network connections/sockets rather than per-file OS file
descriptors, but " +
+ "they can still count toward OS resource limits (ulimit -n).")
+ .intConf
+ .checkValue(_ > 0, "must be a positive number")
+ .createWithDefault(10000)
Review Comment:
The default of 10000 matches the upstream Velox default
(`HiveConfig::numCacheFileHandles()`). It's the maximum LRU *capacity*, not a
count of simultaneously-open descriptors -- the cache evicts
least-recently-used entries when full, and the TTL (default 10 min) further
bounds how long idle handles are retained. Workloads that scan more than 10000
distinct files within the TTL window can lower it, and the cache can be
disabled entirely with `fileHandleCacheEnabled=false`. I'd prefer not to
diverge from the upstream default here; a `ulimit`-aware native clamp would be
a separate, broader change.
--
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]
