vinishjail97 commented on code in PR #861:
URL: https://github.com/apache/incubator-xtable/pull/861#discussion_r3641364305
##########
xtable-core/src/test/java/org/apache/xtable/delta/ITDeltaConversionSource.java:
##########
@@ -829,4 +836,54 @@ private boolean checkIfFileIsRemoved(String activePath,
TableChange tableChange)
.collect(Collectors.toSet());
return filePathsRemoved.contains(activePath);
}
+
+ @Test
+ public void
getTableChangeForCommitReconstructsSnapshotOnceForAppendOnlyBacklog() {
Review Comment:
Nice to have the getSnapshotAt call-count assertion. Would it be worth
adding a two-run case as well — sync, append more commits, sync again —
asserting the second run's backlog doesn't re-include already-synced versions?
That would guard the watermark-advancement behavior, which the current
diff-validation tests don't exercise.
##########
xtable-core/src/main/java/org/apache/xtable/delta/DeltaConversionSource.java:
##########
@@ -103,10 +112,21 @@ public InternalSnapshot getCurrentSnapshot() {
@Override
public TableChange getTableChangeForCommit(Long versionNumber) {
List<Action> actionsForVersion =
getChangesState().getActionsForVersion(versionNumber);
- Snapshot snapshotAtVersion = deltaLog.getSnapshotAt(versionNumber,
Option.empty());
- InternalTable tableAtVersion = tableExtractor.table(snapshotAtVersion,
tableName);
- FileFormat fileFormat =
-
actionsConverter.convertToFileFormat(snapshotAtVersion.metadata().format().provider());
+ // Reconstructing the snapshot per commit reloads the table checkpoint
each time, which is
+ // expensive when the checkpoint is large. Reuse it across the backlog and
reload only when a
+ // commit changes schema or protocol.
+ boolean metadataMayHaveChanged =
+ actionsForVersion.stream()
+ .anyMatch(action -> (action instanceof Metadata) || (action
instanceof Protocol));
+ if (cachedSnapshot == null || metadataMayHaveChanged) {
+ cachedSnapshot = deltaLog.getSnapshotAt(versionNumber, Option.empty());
+ cachedTable = tableExtractor.table(cachedSnapshot, tableName);
+ cachedFileFormat =
+
actionsConverter.convertToFileFormat(cachedSnapshot.metadata().format().provider());
+ }
+ Snapshot snapshotAtVersion = cachedSnapshot;
+ InternalTable tableAtVersion = cachedTable;
Review Comment:
Reuse is fine for schema/partitioning/format/base path — those only change
on Metadata/Protocol, which the reload check handles. The catch is
latestCommitTime: it's the per-commit snapshot.timestamp(), and it gets
persisted as the sync watermark (syncMetadata/lastInstantSynced). With the
cache, every commit in an append-only backlog reports the reload version's
timestamp, so we persist the first version's instant instead of the latest and
next run re-processes the tail from there.
Could we cache the snapshot but set latestCommitTime per commit from
CommitInfo (already in actionsForVersion) or DeltaHistoryManager?
--
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]
