Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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 @@ -129,7 +129,9 @@ class FileStreamSource(
log"maxBytesPerBatch = ${MDC(LogKeys.NUM_BYTES, maxBytesPerBatch)}, " +
log"maxFileAgeMs = ${MDC(LogKeys.TIME_UNITS, maxFileAgeMs)}")

private var unreadFiles: Seq[NewFileEntry] = _

// Visible for testing
private[sql] var unreadFiles: Seq[NewFileEntry] = _

/**
* Split files into a selected/unselected pair according to a total size threshold.
Expand Down Expand Up @@ -179,9 +181,11 @@ class FileStreamSource(
}
}

val shouldCache = !sourceOptions.latestFirst && allFilesForTriggerAvailableNow == null

// Obey user's setting to limit the number of files in this batch trigger.
val (batchFiles, unselectedFiles) = limit match {
case files: ReadMaxFiles if !sourceOptions.latestFirst =>
case files: ReadMaxFiles if shouldCache =>
// we can cache and reuse remaining fetched list of files in further batches
val (bFiles, usFiles) = newFiles.splitAt(files.maxFiles())
if (usFiles.size < files.maxFiles() * DISCARD_UNSEEN_INPUT_RATIO) {
Expand All @@ -195,10 +199,10 @@ class FileStreamSource(
}

case files: ReadMaxFiles =>
// implies "sourceOptions.latestFirst = true" which we want to refresh the list per batch
// don't use the cache, just take files for the next batch
(newFiles.take(files.maxFiles()), null)

case files: ReadMaxBytes if !sourceOptions.latestFirst =>
case files: ReadMaxBytes if shouldCache =>
// we can cache and reuse remaining fetched list of files in further batches
val (FilesSplit(bFiles, _), FilesSplit(usFiles, rSize)) =
takeFilesUntilMax(newFiles, files.maxBytes())
Expand All @@ -213,8 +217,8 @@ class FileStreamSource(
}

case files: ReadMaxBytes =>
// don't use the cache, just take files for the next batch
val (FilesSplit(bFiles, _), _) = takeFilesUntilMax(newFiles, files.maxBytes())
// implies "sourceOptions.latestFirst = true" which we want to refresh the list per batch
(bFiles, null)

case _: ReadAllAvailable => (newFiles, null)
Expand Down Expand Up @@ -436,8 +440,8 @@ object FileStreamSource {
def sparkPath: SparkPath = SparkPath.fromUrlString(path)
}

/** Newly fetched files metadata holder. */
private case class NewFileEntry(path: SparkPath, size: Long, timestamp: Long)
/** Newly fetched files metadata holder. Visible for testing. */
private[sql] case class NewFileEntry(path: SparkPath, size: Long, timestamp: Long)

private case class FilesSplit(files: Seq[NewFileEntry], size: BigInt)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2357,6 +2357,46 @@ class FileStreamSourceSuite extends FileStreamSourceTest {
}
}

test("SPARK-48314: Don't cache unread files when using Trigger.AvailableNow") {
withCountListingLocalFileSystemAsLocalFileSystem {
withThreeTempDirs { case (src, meta, tmp) =>
val options = Map("latestFirst" -> "true", "maxFilesPerTrigger" -> "5")
val scheme = CountListingLocalFileSystem.scheme
val source = new FileStreamSource(spark, s"$scheme:///${src.getCanonicalPath}/*/*", "text",
StructType(Nil), Seq.empty, meta.getCanonicalPath, options)

source.prepareForTriggerAvailableNow()

CountListingLocalFileSystem.resetCount()

// provide 20 files in src, with sequential "last modified" to guarantee ordering
(0 to 19).map { idx =>
val f = createFile(idx.toString, new File(src, idx.toString), tmp)
f.setLastModified(idx * 10000)
f
}

source.latestOffset(FileStreamSourceOffset(-1L), ReadLimit.maxFiles(5))

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.

Shall we check the result just for completeness sake?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated to check files returned, and used the new setting from #45362 to verify correct number of files based on not caching

.asInstanceOf[FileStreamSourceOffset]

// All files are already tracked in allFilesForTriggerAvailableNow
assert(0 === CountListingLocalFileSystem.pathToNumListStatusCalled
.get(src.getCanonicalPath).map(_.get()).getOrElse(0))

// Unread files should be empty
assert(source.unreadFiles == null)

// Reading again leverages the files already tracked in allFilesForTriggerAvailableNow,
// so no more listings need to happen
source.latestOffset(FileStreamSourceOffset(-1L), ReadLimit.maxFiles(5))

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.

Same here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same as above

.asInstanceOf[FileStreamSourceOffset]

assert(0 === CountListingLocalFileSystem.pathToNumListStatusCalled
.get(src.getCanonicalPath).map(_.get()).getOrElse(0))
}
}
}

test("SPARK-31962: file stream source shouldn't allow modifiedBefore/modifiedAfter") {
def formatTime(time: LocalDateTime): String = {
time.format(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss"))
Expand Down