Copilot commented on code in PR #12400:
URL: https://github.com/apache/gluten/pull/12400#discussion_r3560720183


##########
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:
   With file handle caching now enabled by default, a default max size of 
10,000 cached handles can easily exceed typical per-process file descriptor 
limits (often 1024/4096) on local/HDFS workloads, leading to runtime failures 
like "Too many open files" when scanning many distinct files/partitions. 
Consider lowering the default (or making it adaptive to ulimit / executor 
slots) so that the new default-on behavior is safer out of the box, while still 
allowing advanced users to raise it explicitly.



-- 
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]

Reply via email to