feat: PartialSegmentFileMapper no longer lazy loads#19693
Conversation
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
Reviewed 6 of 6 changed files.
Found one lifecycle race: eviction between residency planning and bundle acquisition can leave required files unfetched and make cursor construction fail.
This is an automated review by Codex GPT-5.6-Sol
| ensureFileDownloaded(name, fileMetadata); | ||
| // Mapping never downloads, every caller is expected to have made the file resident through an explicit plan before | ||
| // reading it | ||
| if (!downloadedFiles.contains(name)) { |
There was a problem hiding this comment.
[P1] Acquire bundle holds before planning resident files
makeCursorHolderAsync snapshots residency in planCursorPrefetch before acquiring the corresponding bundle holds. If a resident, currently unheld bundle is evicted in that window, evictContainer clears its downloaded-file entries and the subsequent acquire only remounts sparse containers; files omitted from the original plan are never restored. This new exception then makes holder materialization fail, whereas the removed ensureFileDownloaded path recovered under the hold. Acquire bundle holds before the residency snapshot, or re-plan the fetches after all holds are acquired.
There was a problem hiding this comment.
while fixing this, also noticed a flaw in PartialQueryableIndex that the column holder suppliers are memoized, but do not consider if their underlying bundles have/have not been evicted, so added some 'generation' tracking for the bundles and special suppliers that can check if they were created for the current generation or not.
3 participants
Description
After #19683, all partial load operations are now deliberately planned, so this PR begins to clean some stuff up by removing the lazy load behavior of
mapFile, which prior to this patch would perform a load on demand if the internal file we were trying to map was not present in the segment.mapFilenow throws if the file is not present since lazy loading in this fashion (like, done inline with processing) is unlikely to ever be desired behavior.Eventually I'd like to consolidate all fetching to be the parallel planned fetches, but some of the refactoring required is a bit more involved than this change (e.g. the full-load fallback the native engines use), so will save this for a follow-up.