Cache PositionIndex #5264
Cache PositionIndex #5264
Conversation
|
@rdblue cc |
| if (deleteRowPositions == null) { | ||
| deleteRowPositions = Deletes.toPositionIndex(filePath, deletes); | ||
| } | ||
| Predicate<T> isDeleted = record -> deleteRowPositions.isDeleted(pos(record)); |
There was a problem hiding this comment.
If deleteRowPositions is null, is this predicate at risk of an NPE?
There was a problem hiding this comment.
No NPE. The return of " Deletes.toPositionIndex(filePath, deletes)" must not be null.
| if (posDeletes.stream().mapToLong(DeleteFile::recordCount).sum() < setFilterThreshold) { | ||
| PositionDeleteIndex positionIndex = Deletes.toPositionIndex(filePath, deletes); | ||
| Predicate<T> isDeleted = record -> positionIndex.isDeleted(pos(record)); | ||
| if (deleteRowPositions == null) { |
There was a problem hiding this comment.
I don't think this should run line 225 to open the deletes if they can be reused.
| List<CloseableIterable<Record>> deletes = Lists.transform(posDeletes, this::openPosDeletes); | ||
|
|
||
| // if there are fewer deletes than a reasonable number to keep in memory, use a set | ||
| if (posDeletes.stream().mapToLong(DeleteFile::recordCount).sum() < setFilterThreshold) { |
There was a problem hiding this comment.
What about the else case? In that case we will not cache.
There was a problem hiding this comment.
Yes, it won't cache because the deletes are streamed from files.
There was a problem hiding this comment.
Yes, Because it use sort merge to avoid loading too many delete rows in memory.
There was a problem hiding this comment.
But, They should't reopen position delete file and read position delete file from head, they should read position delete file continue。I will do some change here
|
I add some code to reuse Merge Sort, I don't know whether it is suitable |
4 participants
Like #5195 PositionIndex also can be cache