Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess - #19394
Draft
deepthi912 wants to merge 1 commit into
Draft
Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess#19394deepthi912 wants to merge 1 commit into
deepthi912 wants to merge 1 commit into
Conversation
ImmutableSegmentLoader#needPreprocess already short-circuits when the (possibly tier-scoped) flag is set. Callers on the warm-load and reload paths route through that helper, so they observe the flag correctly. But the cold-download path in BaseTableDataManager — downloadAndLoadSegment and replaceSegmentIfCrcMismatch — invokes the 4-arg ImmutableSegmentLoader#load, whose overload hard-codes needPreprocess=true and calls preprocess directly. Preprocess never consulted the flag, so every index handler (V1→V3 conversion, default columns, inverted, range, and StarTree parquet null-vector / page-index build) ran even when the table config asked to skip. Move the flag check into preprocess itself. Every caller now behaves consistently: the flag is honored uniformly whether preprocess is reached through needPreprocess or directly. Regression test builds a V1 segment, invokes preprocess with skipSegmentPreprocess=true, and asserts that the V1→V3 conversion (which the ILC would otherwise request) does not run.
deepthi912
marked this pull request as draft
August 29, 2026 01:07
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19394 +/- ##
============================================
- Coverage 67.57% 67.57% -0.01%
Complexity 1430 1430
============================================
Files 3486 3486
Lines 224134 224138 +4
Branches 35376 35377 +1
============================================
- Hits 151464 151454 -10
- Misses 60652 60659 +7
- Partials 12018 12025 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ImmutableSegmentLoader#needPreprocessalready short-circuits whenskipSegmentPreprocessis set (and after #19391, honors the tier-scoped override too). Callers on the warm-load and reload paths route throughneedPreprocess, so they observe the flag correctly. But the cold-download path inBaseTableDataManager—downloadAndLoadSegmentandreplaceSegmentIfCrcMismatch— invokes the 4-argImmutableSegmentLoader#loadoverload that hard-codesneedPreprocess=trueand callspreprocessdirectly. Preprocess itself never consulted the flag, so every index handler (V1→V3 conversion, default columns, inverted, range, StarTree parquet null-vector / page-index build) ran even when the table config asked to skip.This PR moves the flag check into
preprocessitself. Every caller now behaves consistently — whether preprocess is reached throughneedPreprocessor directly,skipSegmentPreprocess=trueresults in a uniform no-op.Changes
ImmutableSegmentLoader.preprocess: early-return whenindexLoadingConfig.isSkipSegmentPreprocess()is true. Log at INFO so the skip is auditable.Related