Core: Support IncrementalChangelogScan with deletes. #6182
Conversation
|
I had a local PoC for this which was blocked by some changes around sequence numbers. I'll try to find some time to review this PR later this week. |
|
@Reo-LEI @aokolnychyi any updates on this one? Would be great to get this in |
|
Sorry for the delay on this one. I will get to it as soon as I can. I wanted to make sure we wrap v1 table support. |
| containAddedDeleteFiles = containAddedDeleteFiles || !info.addedDeleteFiles().isEmpty(); | ||
| containRemovedDataFiles = containRemovedDataFiles || info.hasRemovedDataFiles(); |
There was a problem hiding this comment.
You could short-circuit this loop if both are true. Only matters if the set of snapshot infos may be large though.
| .filterManifestEntries(entry -> changelogSnapshotIds.contains(entry.snapshotId())) | ||
| .ignoreExisting(); |
There was a problem hiding this comment.
Were these two lines redundant, or are they no longer needed for another reason?
| Set<ManifestFile> deleteManifests; | ||
| if (containAddedDeleteFiles) { | ||
| // scan all dataFiles to locate the deleted record and ensure that this record has not been deleted before | ||
| dataManifests = Sets.newHashSet(changelogSnapshots.getLast().dataManifests(tableOps().io())); |
There was a problem hiding this comment.
Why can you get away with just the last snapshot in the queue here, but in the other branch you have to iterator over all of them?
| context.specAsString(), | ||
| context.residuals()); | ||
| } else { | ||
| // ignore removed data files. |
There was a problem hiding this comment.
This case where the commit snapshot id doesn't exist in snapshotOrdinals didn't seem to be possible before, what changed?
|
@Reo-LEI and @aokolnychyi, are you still working on this? |
Sorry for the late reply. I haven't been doing this recently. You can continue to push forward. @manuzhang |
|
I just submitted #9888 |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
This PR is trying to support
IncrementalChangelogScanwith deletes after #5382Any feedback is welcome！
cc @rdblue @aokolnychyi @flyrain @stevenzwu @kbendick @chenjunjiedada