Core: Parallelize deletefileindex loading in MergingSnapshotProducer - #18156
grantatspothero wants to merge 1 commit into
Conversation
e5b0cc1 to
4921ee5
Compare
grantatspothero
changed the title
| .caseSensitive(caseSensitive) | ||
| .specsById(ops().current().specsById()); | ||
| .specsById(ops().current().specsById()) | ||
| .planWith(workerPool()); |
There was a problem hiding this comment.
This is parallelizing delete manifest reads within a single DeleteFileIndex build (i.e. useful when there are many delete manifests added in a given snapshot).
But if we want to optimize the case where many snapshots were added, we would want to parallelize the creation of all these indices
.I think optimizing for the latter is more useful because reducing validation latency is more useful for things like compaction jobs trying to keep up with streaming workloads where the number of delete manifests that should be produced per commit should be small to begin with (and even for big batch jobs, we'd be merging delete manifests etc) so parallelism at that level in validation probably isn't that useful.
There was a problem hiding this comment.
Agree with you that parallelizing across snapshots is better.
I made this change because it is trivially correct and better than the existing implementation. Let me explore parallelizing across snapshots
There was a problem hiding this comment.
One thing that came up is reentrancy of parallelizing tasks using the worker pool. See discussion here: #18076 (comment)
In this example, it means we cannot parallelize both across snapshots and within a snapshot due to reentrancy and deadlock. So you have to choose which level to parallelize at, and I think I agree with you parallelizing across snapshots is better.
Is this reentrancy issue something you have seen elsewhere in Iceberg?
2 participants
validateNoNewDeletesForDataFilesis expensive when there are many snapshots between thestartingSnapshotIdand the commit HEAD.Parallelizes the deletefileindex building using the worker pool.