Skip to content
Open
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
f0a2d13
[VL] Enable file handle cache by default with TTL-based eviction
iemejia Jun 29, 2026
92ffa22
Address review comments: fix schema assertion, deletion test, patch g…
iemejia Jun 30, 2026
b7f6f4b
Tighten fileCount assertion to >= 200 to match repartition(200)
iemejia Jun 30, 2026
3f877e6
Add TTL eviction test: verify scans succeed after cached handles expire
iemejia Jun 30, 2026
abbd63e
Narrow catch to file-not-found errors; use count() instead of collect…
iemejia Jun 30, 2026
ba9f6b0
Regenerate configuration docs for new file handle cache configs
iemejia Jun 30, 2026
4888b82
Add value validation for numCacheFileHandles and fileHandleExpiration…
iemejia Jun 30, 2026
53c916c
Clarify that remote FS cache entries still count toward OS limits
iemejia Jul 8, 2026
588c9ca
Add checkGlutenPlan assertions to all file handle cache tests
iemejia Jul 8, 2026
c916013
Fix config doc padding and address review comments
iemejia Jul 8, 2026
e09869f
Remove trailing whitespace; reword cache-hit comments
iemejia Jul 9, 2026
f681771
Fix spark-submit example in FileHandleCacheBenchmark scaladoc
iemejia Jul 9, 2026
a12bf85
Add file handle cache configs to native conf defaults list
iemejia Jul 9, 2026
ab998da
Use standard a/b prefixes in file-handle-cache-ttl.patch
iemejia Jul 9, 2026
4e6fe69
Rename repeated-scan test to drop 'cache hit path' claim
iemejia Jul 9, 2026
ba400ea
Use timeConf for fileHandleExpirationDurationMs
iemejia Jul 9, 2026
51ed164
Extract TTL constant; remove remaining 'cache hit path' comments
iemejia Jul 9, 2026
4a9593a
Add checkValue validation for ssdCacheIOThreads
iemejia Jul 9, 2026
03b24cd
Revert timeConf back to longConf for fileHandleExpirationDurationMs
iemejia Jul 9, 2026
166d04d
Use timeConf for fileHandleExpirationDurationMs
iemejia Jul 10, 2026
477d5c4
Quote path expansions in apply_compilation_fixes for safety
iemejia Jul 10, 2026
7bdc456
Make apply_compilation_fixes self-contained; clarify TTL test scope
iemejia Jul 10, 2026
5daba68
Harden deletion test: check cause chain before message matching
iemejia Jul 10, 2026
4d00c7c
Rename TTL test to reflect what it actually asserts
iemejia Jul 10, 2026
410e22f
Fix scalafmt: move case guard to its own line
iemejia Jul 10, 2026
1d2d4f7
Drop file-handle-cache-ttl.patch; change is upstream in Velox 2026_07_16
iemejia Jul 19, 2026
a7679cf
Merge branch 'main' into feature/velox-enable-file-handle-cache-default
jackylee-ch Jul 20, 2026
0e3784a
Run file handle cache tests on all Spark versions
iemejia Jul 20, 2026
66f5022
Reduce file handle cache TTL test sleep to speed up CI
iemejia Jul 20, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -180,9 +180,10 @@ object VeloxConfig extends ConfigRegistry {

val COLUMNAR_VELOX_SSD_CACHE_IO_THREADS =
buildStaticConf("spark.gluten.sql.columnar.backend.velox.ssdCacheIOThreads")
.doc("The IO threads for cache promoting")
.doc("The number of IO threads for SSD cache read/write operations")
.intConf
.createWithDefault(1)
.checkValue(_ > 0, "must be a positive number")
.createWithDefault(4)
Comment thread
jackylee-ch marked this conversation as resolved.
Comment thread
jackylee-ch marked this conversation as resolved.

val COLUMNAR_VELOX_SSD_ODIRECT_ENABLED =
buildStaticConf("spark.gluten.sql.columnar.backend.velox.ssdODirect")
Expand Down Expand Up @@ -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)
Comment thread
jackylee-ch marked this conversation as resolved.
Comment thread
jackylee-ch marked this conversation as resolved.
Comment thread
iemejia marked this conversation as resolved.

Comment thread
jackylee-ch marked this conversation as resolved.
val COLUMNAR_VELOX_FILE_HANDLE_EXPIRATION_DURATION_MS =
buildStaticConf("spark.gluten.sql.columnar.backend.velox.fileHandleExpirationDurationMs")
.doc(
"Expiration time in milliseconds for cached file handles. Handles not accessed " +
"within this duration are evicted from the cache. This prevents stale handles " +
"from accumulating (e.g., expired HDFS leases, closed remote connections). " +
"A value of 0 disables TTL-based eviction.")
.timeConf(TimeUnit.MILLISECONDS)
.checkValue(_ >= 0, "must be a non-negative number (0 disables TTL-based eviction)")
.createWithDefault(600000L) // 10 minutes
Comment thread
jackylee-ch marked this conversation as resolved.
Comment thread
jackylee-ch marked this conversation as resolved.

val DIRECTORY_SIZE_GUESS =
buildStaticConf("spark.gluten.sql.columnar.backend.velox.directorySizeGuess")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,342 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.spark.sql.execution

import org.apache.gluten.config.VeloxConfig
import org.apache.gluten.execution.{BasicScanExecTransformer, VeloxWholeStageTransformerSuite}

import org.apache.spark.SparkConf

import java.io.FileNotFoundException
import java.nio.file.NoSuchFileException

/**
* Test suite for Velox file handle cache behavior.
*
* Tests correctness, config propagation, and edge cases for the file handle cache which caches open
* file handles (descriptors) to avoid repeated open/close overhead.
*/
class VeloxFileHandleCacheSuite extends VeloxWholeStageTransformerSuite {
override protected val resourcePath: String = "/parquet-for-read"
override protected val fileFormat: String = "parquet"

// TTL for file handle cache eviction (used in sparkConf and sleep calculations)
private val ttlMs = 2000
private val ttlWaitMs = ttlMs + 1000 // TTL + buffer for eviction to take effect

/** Walks the exception cause chain looking for an instance of the given type. */
private def hasCauseOfType(e: Throwable, cls: Class[_ <: Throwable]): Boolean = {
var cause = e.getCause
while (cause != null) {
if (cls.isInstance(cause)) return true
cause = cause.getCause
}
false
}

override protected def sparkConf: SparkConf = {
super.sparkConf
.set(VeloxConfig.COLUMNAR_VELOX_FILE_HANDLE_CACHE_ENABLED.key, "true")
.set(VeloxConfig.COLUMNAR_VELOX_FILE_HANDLE_EXPIRATION_DURATION_MS.key, ttlMs.toString)
.set(VeloxConfig.COLUMNAR_VELOX_NUM_CACHE_FILE_HANDLES.key, "10000")
}
Comment on lines +54 to +59

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Set the suite-level TTL to 2 seconds and added a dedicated test that scans files, waits 3 seconds for handle expiration, then verifies that subsequent scans still return correct results after handles are evicted and re-opened.

Comment thread
jackylee-ch marked this conversation as resolved.

testWithSpecifiedSparkVersion(
"basic scan correctness with file handle cache enabled",
"3.5",
"3.5") {
// Verify that enabling file handle cache produces correct scan results
withTempPath {
dir =>
spark
.range(10000)
.selectExpr("id", "cast(id % 7 as int) as category", "id * 1.5 as value")
.repartition(10)
.write
.parquet(dir.getCanonicalPath)

val df = spark.read.parquet(dir.getCanonicalPath)
df.createOrReplaceTempView("t")

runQueryAndCompare("SELECT count(*) FROM t") {
checkGlutenPlan[BasicScanExecTransformer]
}
runQueryAndCompare("SELECT sum(value) FROM t WHERE category = 3") {
checkGlutenPlan[BasicScanExecTransformer]
}
runQueryAndCompare("SELECT category, count(*) FROM t GROUP BY category") {
checkGlutenPlan[BasicScanExecTransformer]
}
}
}

testWithSpecifiedSparkVersion(
"repeated scans produce consistent results",
"3.5",
"3.5") {
// Repeated scans of the same files must produce identical results regardless
// of whether handles are served from cache or re-opened after TTL eviction.
withTempPath {
dir =>
spark
.range(5000)
.selectExpr("id", "cast(id as string) as name")
.repartition(50) // 50 files to exercise many cache entries
.write
.parquet(dir.getCanonicalPath)

val path = dir.getCanonicalPath
val expected = spark.read.parquet(path).count()
assert(expected == 5000)

// Verify scans go through Gluten/Velox
checkGlutenPlan[BasicScanExecTransformer](spark.read.parquet(path))

// Scan the same files multiple times - results must be consistent
for (i <- 1 to 5) {
val count = spark.read.parquet(path).count()
assert(
count == expected,
s"Iteration $i: expected $expected rows but got $count")
}

// Verify aggregation consistency across repeated scans
val firstSum = spark.read.parquet(path).selectExpr("sum(id)").collect()(0).getLong(0)
for (i <- 1 to 3) {
val sum = spark.read.parquet(path).selectExpr("sum(id)").collect()(0).getLong(0)
assert(
sum == firstSum,
s"Iteration $i: sum mismatch, expected $firstSum but got $sum")
}
Comment thread
jackylee-ch marked this conversation as resolved.
}
}

testWithSpecifiedSparkVersion(
"many small files do not cause errors with file handle cache",
"3.5",
"3.5") {
// Verify that scanning many small files with caching enabled does not cause
// file descriptor exhaustion or other resource-related errors.
withTempPath {
dir =>
// Create 200 small parquet files
spark
.range(20000)
.selectExpr("id", "uuid() as payload")
.repartition(200)
.write
.parquet(dir.getCanonicalPath)

val fileCount = dir.listFiles().count(_.getName.endsWith(".parquet"))
assert(fileCount >= 200, s"Expected at least 200 files, got $fileCount")

// Verify scans go through Gluten/Velox
checkGlutenPlan[BasicScanExecTransformer](spark.read.parquet(dir.getCanonicalPath))

// Scan all files - should work without resource errors
val count = spark.read.parquet(dir.getCanonicalPath).count()
assert(count == 20000)

// Scan again - results must remain consistent
val count2 = spark.read.parquet(dir.getCanonicalPath).count()
assert(count2 == 20000)
Comment on lines +144 to +150

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Added checkGlutenPlan[BasicScanExecTransformer] before the count assertions to confirm scans go through the native path.

}
}

testWithSpecifiedSparkVersion(
"filtered scan correctness with file handle cache",
"3.5",
"3.5") {
// Verify that predicate pushdown works correctly with cached file handles.
// This exercises the row group skipping path through cached handles.
withTempPath {
dir =>
spark
.range(100000)
.selectExpr(
"id",
"cast(id % 10 as int) as partition_key",
"cast(id * 0.01 as double) as metric")
.repartition(20)
.write
.parquet(dir.getCanonicalPath)

val path = dir.getCanonicalPath

// Verify scans go through Gluten/Velox
checkGlutenPlan[BasicScanExecTransformer](
spark.read.parquet(path).where("partition_key = 5"))

// Filter that matches ~10% of rows
val filtered = spark.read.parquet(path).where("partition_key = 5").count()
assert(filtered == 10000, s"Expected 10000 filtered rows, got $filtered")

// Range filter
val rangeFiltered = spark.read.parquet(path).where("id >= 50000").count()
assert(rangeFiltered == 50000, s"Expected 50000 range-filtered rows, got $rangeFiltered")

// Re-run same filters - results must remain consistent
val filtered2 = spark.read.parquet(path).where("partition_key = 5").count()
assert(filtered2 == filtered, "Filtered count mismatch on repeated scan")
Comment thread
jackylee-ch marked this conversation as resolved.
}
}

testWithSpecifiedSparkVersion(
"scan after file deletion does not silently return wrong data",
"3.5",
"3.5") {
// If a file is deleted between scans, the next scan should either:
// - Succeed with the original count (cached FD keeps inode alive on Linux)
// - Succeed with a reduced count (deleted file not accessible)
// - Throw a file-not-found error
// The key invariant: it must NOT silently return incorrect data.
withTempPath {
dir =>
spark
.range(1000)
.selectExpr("id")
.repartition(5)
.write
.parquet(dir.getCanonicalPath)

val path = dir.getCanonicalPath
// First scan populates the cache
val count1 = spark.read.parquet(path).count()
assert(count1 == 1000)

// Verify scans go through Gluten/Velox
checkGlutenPlan[BasicScanExecTransformer](spark.read.parquet(path))

// Delete one parquet file
val parquetFiles = dir.listFiles().filter(_.getName.endsWith(".parquet"))
assert(parquetFiles.nonEmpty)
val deletedFile = parquetFiles.head
val deletedRows = spark.read.parquet(deletedFile.getCanonicalPath).count()
assert(deletedFile.delete(), s"Failed to delete ${deletedFile.getCanonicalPath}")

// On Linux, the cached FD to the deleted file may still work (unlinked inode).
// Either way, the remaining files should be readable.
// The scan may also throw if the FS detects the missing file.
try {
val count2 = spark.read.parquet(path).count()
Comment thread
jackylee-ch marked this conversation as resolved.
// The count should be either (count1 - deletedRows) or count1
// depending on whether the OS kept the inode accessible
assert(
count2 == count1 || count2 == count1 - deletedRows,
s"Unexpected count after deletion: $count2 (original: $count1, deleted: $deletedRows)")
} catch {
case e: FileNotFoundException =>
// Direct file-not-found exception.
case e: NoSuchFileException =>
// NIO equivalent of FileNotFoundException.
case e: Exception
if hasCauseOfType(e, classOf[FileNotFoundException]) ||
hasCauseOfType(e, classOf[NoSuchFileException]) =>
// Wrapped file-not-found in the cause chain (e.g., SparkException wrapping).
case e: Exception
if e.getMessage != null &&
(e.getMessage.contains("FileNotFoundException") ||
e.getMessage.contains("No such file") ||
e.getMessage.contains("Path does not exist") ||
e.getMessage.contains("does not exist")) =>
// Fallback: message-based matching for FS implementations that use
// custom exception types (e.g., Hadoop, Velox native errors).
}
Comment thread
jackylee-ch marked this conversation as resolved.
Comment thread
jackylee-ch marked this conversation as resolved.
}
}

testWithSpecifiedSparkVersion(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should be version-agnostic, we can simply use test instead of testWithSpecifiedSparkVersion.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 0e3784a63. Replaced testWithSpecifiedSparkVersion(..., "3.5", "3.5") with plain test(...) for all tests in the suite — the file handle cache behavior is version-agnostic, so there's no reason to gate these to Spark 3.5. Verified locally against a full native Velox build: all 7 tests pass.

"scans remain correct after TTL expiration window",
"3.5",
"3.5") {
// Correctness guard: verify that scans produce correct results after the
// configured TTL (2s, set in sparkConf) has elapsed and cached handles may
// have been evicted. This does NOT directly assert that eviction occurred
// (Velox exposes no JVM-visible eviction counter), but it exercises the
// re-open path: if a handle was evicted, the scan must transparently
// re-open the file and return the same data. Combined with the "scan after
// file deletion" test -- which proves cached handles keep the inode alive --
// this gives reasonable coverage that the TTL wiring works end-to-end.
withTempPath {
dir =>
spark
.range(5000)
.selectExpr("id", "id * 2 as doubled")
.repartition(20)
.write
.parquet(dir.getCanonicalPath)

val path = dir.getCanonicalPath

// First scan populates the cache
val count1 = spark.read.parquet(path).count()
assert(count1 == 5000)

// Verify scans go through Gluten/Velox
checkGlutenPlan[BasicScanExecTransformer](spark.read.parquet(path))

val sum1 = spark.read.parquet(path).selectExpr("sum(id)").collect()(0).getLong(0)

// Wait for TTL to expire
Thread.sleep(ttlWaitMs)
Comment thread
jackylee-ch marked this conversation as resolved.

// Scan after TTL expiration: verify results remain correct
// (handles may have been evicted and transparently re-opened)
val count2 = spark.read.parquet(path).count()
assert(count2 == 5000, s"Count mismatch after TTL expiration: expected 5000, got $count2")
val sum2 = spark.read.parquet(path).selectExpr("sum(id)").collect()(0).getLong(0)
assert(sum2 == sum1, s"Sum mismatch after TTL expiration: expected $sum1, got $sum2")
Comment on lines +268 to +287

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Added checkGlutenPlan[BasicScanExecTransformer] after the initial scan to ensure the TTL test actually exercises native file-handle caching.

Comment thread
jackylee-ch marked this conversation as resolved.
}
}

testWithSpecifiedSparkVersion(
"column pruning with cached file handles",
"3.5",
"3.5") {
// Verify that column pruning works correctly when file handles are cached.
// The cache key includes the file path but not the projected columns, so
// different projections on the same file must still work correctly.
withTempPath {
dir =>
spark
.range(5000)
.selectExpr("id", "id * 2 as doubled", "id * 3 as tripled", "uuid() as text")
.repartition(10)
.write
.parquet(dir.getCanonicalPath)

val path = dir.getCanonicalPath

// Verify scans go through Gluten/Velox
checkGlutenPlan[BasicScanExecTransformer](spark.read.parquet(path))

// Read all columns
val allCols = spark.read.parquet(path).select("id", "doubled", "tripled", "text").count()
assert(allCols == 5000)

// Read subset of columns (same file handles, different projection)
val subset1Df = spark.read.parquet(path).select("id")
assert(subset1Df.schema.fieldNames.sameElements(Array("id")))
assert(subset1Df.count() == 5000)

Comment thread
jackylee-ch marked this conversation as resolved.
// Different subset
val subset2 = spark.read.parquet(path).selectExpr("sum(doubled)").collect()
val expectedSum = (0L until 5000L).map(_ * 2).sum
assert(subset2(0).getLong(0) == expectedSum)
Comment thread
jackylee-ch marked this conversation as resolved.
}
}
}
Loading
Loading