AadhiKat commented on code in PR #861:
URL: https://github.com/apache/incubator-xtable/pull/861#discussion_r3641884400
##########
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:
Thanks @vinishjail97. You're right that with the snapshot cached the
watermark would sit at the reload version and the next run would re-read the
tail.
Fixed by reading `latestCommitTime` per commit from the commit's `CommitInfo`
(already in `actionsForVersion`, so no extra checkpoint read), falling back
to
`snapshot.timestamp()` only if a commit has no `CommitInfo`. Everything else
still
comes from the cached snapshot, since it doesn't change between commits.
One thing worth flagging: `CommitInfo.timestamp` is the writer's time rather
than
the commit-file mtime that `snapshot.timestamp()` uses, so at a backlog
boundary the
next run could re-touch the final commit. It's idempotent, but if you'd
rather keep
it exact I can read the timestamp from `DeltaHistoryManager` instead. Happy
either way.
--
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]
