-
Notifications
You must be signed in to change notification settings - Fork 3k
0.12.1 Cherry-Picks: Part 2 #3429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
0.12.1 Cherry-Picks: Part 2 #3429
Conversation
…e#3071) This change optimizes our check for referenced data files in BaseRowDelta by pushing down the conflict detection filter. Previously, we would open manifests even though they belonged to partitions out of our interest.
|
This should be included before #3428. This also includes, but doesn't need #3264, just as #3428 does. I added it to try to make the test suite run faster yesterday as it was really slow (maybe just noisy neighbors on the runners). Can remove on rebase. Lastly, this includes #3071, which wasn't marked for inclusion but made the whole process much simpler. If we don't want it, I can relatively easily remove it. |
| this.validateNewDataFiles = true; | ||
| failMissingDeletePaths(); | ||
| return this; | ||
| } | ||
|
|
||
| @Override | ||
| public OverwriteFiles validateNoConflictingDeletes() { | ||
| this.validateNewDeleteFiles = true; | ||
| failMissingDeletePaths(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that both of these have failMissingDeletePaths makes me think something happened with the cherry-picks. Investigating further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this is the way both of these functions look in master - Is this intentional?:
iceberg/core/src/main/java/org/apache/iceberg/BaseOverwriteFiles.java
Lines 100 to 111 in fad77e8
| public OverwriteFiles validateNoConflictingData() { | |
| this.validateNewDataFiles = true; | |
| failMissingDeletePaths(); | |
| return this; | |
| } | |
| @Override | |
| public OverwriteFiles validateNoConflictingDeletes() { | |
| this.validateNewDeleteFiles = true; | |
| failMissingDeletePaths(); | |
| return this; | |
| } |
Wondering if this is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind. This looks correct. Here's the actual function called:
iceberg/core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
Lines 143 to 146 in fad77e8
| protected void failMissingDeletePaths() { | |
| filterManager.failMissingDeletePaths(); | |
| deleteFilterManager.failMissingDeletePaths(); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is intentional. This was to preserve the old behavior.
|
Looks great! I merged these into 0.12.x. Thanks, @kbendick! |
This PR is to get the following two PRs, slated for release in 0.12.1, to be able to merge.
Neither of them cherry-pick cleanly, though I do know the set of PRs that would be needed to do so (as mentioned in another PR).
** Merge first - After a decision is made about #3264