AadhiKat commented on code in PR #861:
URL: https://github.com/apache/incubator-xtable/pull/861#discussion_r3735173778
##########
xtable-core/src/main/java/org/apache/xtable/delta/DeltaConversionSource.java:
##########
@@ -78,6 +81,13 @@ public class DeltaConversionSource implements
ConversionSource<Long> {
private final String tableName;
private final String basePath;
+ // Cache the version-stable snapshot/table/format across an incremental
backlog to avoid
+ // reconstructing the Delta snapshot (a full checkpoint read) on every
commit. Invalidated
+ // whenever a commit carries a metadata/protocol change (see
getTableChangeForCommit).
+ private Snapshot cachedSnapshot;
Review Comment:
Done, all four — pushed as 48ef6d7.
**Metadata cache.** As you sketched it: single `cachedMetadata` field,
baseline from one
`getSnapshotAt().metadata()` (snapshot not retained), refreshed from the
commit's own `metaData`
action, last-in-commit wins. `Protocol` left the guard.
`DeltaTableExtractor` gained the `Metadata`
overload; the actions converter got base-path variants alongside the
`Snapshot` ones (which now
delegate), same shape as the kernel converter.
**Timestamp.** You're right, and this was the sharpest catch of the round —
thanks. `CommitInfo.timestamp`
is the writer's clock, `getCommitsBacklog` resolves the watermark against
mtimes, so a fast writer
clock could push the watermark past commits that were never processed.
Skipped data, not
reprocessed data — worse than what the round-1 fix was defending against.
`DeltaIncrementalChangesState`
now carries per-version mtimes from `getChangeLogFiles` (populated in the
constructor next to the
existing `getChanges` pass) and exposes `getCommitTimestamp(version)`;
`DeltaConversionSource`'s
CommitInfo path and its reconstruction fallback are both gone.
**Config.** `xtable.delta.source.reuse_metadata_across_commits`, read from
`sourceTable.getAdditionalProperties()` in the provider, default true.
`false` restores per-commit
snapshot reconstruction exactly.
**Tests.** You were right about `testAddColumns` —
`includeAdditionalColumns` is a creation-time
flag, so no `metaData` action ever fires mid-backlog, and
`ITConversionController` builds a fresh
source per sync so its cache is always cold. Added
`schemaChangeMidBacklogRefreshesCachedMetadataWithoutSnapshotReload`: one
source instance, backlog of
append → `ALTER TABLE ADD COLUMNS` → append, asserting the pre-alter commit
lacks the column, the
alter and post-alter commits carry it, and — since the refresh comes from
the action —
`getSnapshotAt` is still called exactly once. Also added
`reuseMetadataDisabledReconstructsSnapshotPerCommit` pinning the flag-off
path to one reconstruction
per commit.
Re-ran the A/B from the earlier comment on the rebuilt jar (same table, same
protocol; it has
grown ~300 versions since, so reloads run slightly heavier now):
```
RELOAD (cache cleared per commit) run 1 : 110,760 ms / 6 commits (genuine
reloads 21.6-22.5 s)
REUSE (default) : 21,316 ms / 6 commits (baseline
21.2 s, then 3-24 ms)
RELOAD (cache cleared per commit) run 2 : 108,836 ms / 6 commits
```
(The RELOAD arm nulls `cachedMetadata` before each commit, which takes the
same branch the config
flag disables — the flag-off path is additionally pinned by its own test.)
Same shape as before —
the metadata cache didn't change the win. One new cost to be upfront about:
backlog init now does the extra
`getChangeLogFiles` listing for the mtimes. Measured three runs on each
build: 22.1-24.5 s init on
this branch vs 17.1-28.1 s on the previous one — the added pass is inside
run-to-run noise on GCS
(init is dominated by `getActiveCommitAtTime` plus the `getChanges` read
either way), and it's paid
once per sync rather than per commit.
The schema-change case isn't in the table because I won't ALTER this
production table for a
benchmark — but it's pinned by the new test: the refresh comes from the
commit's own action, so a
schema change mid-backlog that previously cost a full reload (~21 s here)
now costs no snapshot
read at all (`getSnapshotAt` asserted to run exactly once across the
evolve-mid-backlog run).
Full `mvn clean install` is green (xtable-core: 486 unit / 126 IT, 0
failures). One note if you
re-run the repro I posted earlier: its reflection cleared the old field
names — updated it to
handle both layouts.
Two notes on the diff so you don't have to ask:
- `DeltaIncrementalChangesState` now lists the log twice per backlog
(`getChanges` + `getChangeLogFiles`), and since `getChanges` is itself built on
`getChangeLogFiles`, one pass could gather actions and mtimes together. I kept
the two calls because the one-pass version means reimplementing Delta's action
parsing on our side, and the second listing measured within run-to-run noise
(numbers above) — but if you'd rather have the single pass, it's mechanical and
I'm happy to do it here.
- `DeltaKernelConversionSource.getTableChangeForCommit` has the same
per-commit reconstruction (`table.getSnapshotAsOfVersion` per commit). I'd
rather not grow this PR's scope — if you agree, I'll file a follow-up issue to
bring the same metadata reuse to the kernel path and take it.
--
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]
