[Kernel] Further refactor and cleanup SnapshotManager #4090
Conversation
kernel/kernel-api/src/test/scala/io/delta/kernel/internal/SnapshotManagerSuite.scala
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
| final CheckpointInstance maxCheckpoint = | ||
| versionToLoadOpt.map(CheckpointInstance::new).orElse(CheckpointInstance.MAX_VALUE); | ||
| logger.debug("lastCheckpoint: {}", maxCheckpoint); | ||
| final List<FileStatus> listedCheckpointFileStatuses = listedCheckpointAndDeltaFileStatuses._1; |
There was a problem hiding this comment.
nit: I don't think we need the listed prefix for all these variables (these plus those above); seems clear enough and can shorten them all a bit
There was a problem hiding this comment.
I actually disagree. There are the checkpoint file statuses we have listed and then there's the latest+complete checkpoint file statuses that we end up selecting. I think emphasizing that these are the listed ones -- but that doesn't mean they are the ones we are selecting to be in the log segmnt -- is important.
See below:
////////////////////////////////////////////////////////////////////////////////////////////
// Step 10: Grab the actual checkpoint file statuses for latestCompleteCheckpointVersion. //
////////////////////////////////////////////////////////////////////////////////////////////
final List<FileStatus> latestCompleteCheckpointFileStatuses =
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
| def createMockFSAndJsonEngineForLastCheckpoint( | ||
| contents: Seq[FileStatus], lastCheckpointVersion: Optional[java.lang.Long]): Engine = { | ||
| mockEngine( | ||
| fileSystemClient = new MockListFromFileSystemClient(listFromProvider(contents)), | ||
| jsonHandler = if (lastCheckpointVersion.isPresent) { | ||
| new MockReadLastCheckpointFileJsonHandler( | ||
| s"$logPath/_last_checkpoint", | ||
| lastCheckpointVersion.get() | ||
| ) | ||
| } else { | ||
| null | ||
| } | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Any reason to put this here instead of with the MockReadLastCheckpointFileJsonHandler?
There was a problem hiding this comment.
This is next to all the other createMockFS____ utilities, thought it would be best here
kernel/kernel-api/src/test/scala/io/delta/kernel/internal/SnapshotManagerSuite.scala
Outdated
Show resolved
Hide resolved
scottsand-db
force-pushed
the
kernel_snapshot_manager_refactor_4
branch
from
6ccd651 to
1f489b3
Compare
Which Delta project/connector is this regarding?
Description
This PR further refactors and cleans up SnapshotManager. We consolidate all of our
getLogSegmentmethods to just one. It has a deep implementation. This PR adds nice, clean header blocks to describe the steps during log segment construction.How was this patch tested?
Refactor. Update existing UTs.
Does this PR introduce any user-facing changes?
No.