JingsongLi commented on code in PR #9048:
URL: https://github.com/apache/paimon/pull/9048#discussion_r3721768837
##########
paimon-core/src/main/java/org/apache/paimon/table/source/snapshot/TimeTravelUtil.java:
##########
@@ -251,17 +255,36 @@ private static Snapshot changelogOrSnapshot(
}
public static void checkRescaleBucketForIncrementalDiffQuery(
- SchemaManager schemaManager, Snapshot start, Snapshot end) {
- if (start.schemaId() != end.schemaId()) {
- int startBucketNumber = bucketNumber(schemaManager,
start.schemaId());
- int endBucketNumber = bucketNumber(schemaManager, end.schemaId());
- if (startBucketNumber != endBucketNumber) {
- throw new InconsistentTagBucketException(
- start.id(),
- end.id(),
- String.format(
- "The bucket number of two snapshots are
different (%s, %s), which is not supported in incremental diff query.",
- startBucketNumber, endBucketNumber));
+ SchemaManager schemaManager, SnapshotReader reader, Snapshot
start, Snapshot end) {
+ TableSchema schema = schemaManager.latest().get();
+ if (!schema.primaryKeys().isEmpty() && schema.numBuckets() ==
BucketMode.POSTPONE_BUCKET) {
+ Map<BinaryRow, Integer> startBucketNumbers =
+ PostponeUtils.getKnownNumBuckets(reader, start.id());
Review Comment:
**[P1] Preserve tag snapshots after snapshot expiration**
`start` and `end` may come from `Tag.trimToSnapshot()`, which is what allows
tag reads to continue after the original snapshots have been expired. Passing
only `start.id()` here eventually calls `reader.withSnapshot(long)` and reloads
`snapshot-N` from the main snapshot directory. Once that file is expired, this
validation throws before the tag diff can run.
I reproduced this with a `bucket = -2` table: create `TAG1`/`TAG2`, retain
only the latest snapshot, then query the diff between the tags; it fails with
`snapshot-1 does not exist`. Please pass the `Snapshot` object through
`getKnownNumBuckets` and call `reader.withSnapshot(snapshot)`. It would also be
useful to add a regression test that expires the source snapshots while
retaining the tags.
##########
paimon-core/src/main/java/org/apache/paimon/table/source/snapshot/TimeTravelUtil.java:
##########
@@ -251,17 +255,36 @@ private static Snapshot changelogOrSnapshot(
}
public static void checkRescaleBucketForIncrementalDiffQuery(
- SchemaManager schemaManager, Snapshot start, Snapshot end) {
- if (start.schemaId() != end.schemaId()) {
- int startBucketNumber = bucketNumber(schemaManager,
start.schemaId());
- int endBucketNumber = bucketNumber(schemaManager, end.schemaId());
- if (startBucketNumber != endBucketNumber) {
- throw new InconsistentTagBucketException(
- start.id(),
- end.id(),
- String.format(
- "The bucket number of two snapshots are
different (%s, %s), which is not supported in incremental diff query.",
- startBucketNumber, endBucketNumber));
+ SchemaManager schemaManager, SnapshotReader reader, Snapshot
start, Snapshot end) {
+ TableSchema schema = schemaManager.latest().get();
+ if (!schema.primaryKeys().isEmpty() && schema.numBuckets() ==
BucketMode.POSTPONE_BUCKET) {
+ Map<BinaryRow, Integer> startBucketNumbers =
+ PostponeUtils.getKnownNumBuckets(reader, start.id());
+ Map<BinaryRow, Integer> endBucketNumbers =
+ PostponeUtils.getKnownNumBuckets(reader, end.id());
Review Comment:
**[P2] Avoid scanning both snapshots twice**
These two calls traverse the active manifest entries for the start and end
snapshots, and `readIncrementalDiff` immediately plans both snapshots again.
Therefore every postpone-bucket diff whose bucket counts are unchanged performs
four manifest traversals instead of two; without a manifest cache this also
duplicates the remote manifest reads.
Could the bucket counts be derived from the `beforeFiles` and `afterFiles`
already collected by `readIncrementalDiff`, or otherwise be returned from the
same planning pass?
--
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]
