deepthi912 commented on code in PR #19394:
URL: https://github.com/apache/pinot/pull/19394#discussion_r3911344632
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -96,13 +96,14 @@ public static ImmutableSegment load(File indexDir,
IndexLoadingConfig indexLoadi
return load(indexDir, indexLoadingConfig, true,
segmentOperationsThrottlerSet, null);
}
- /// Loads the segment with specified IndexLoadingConfig.
- /// This method modifies the segment like to convert segment format, add or
remove indices.
- /// Mostly used by UT cases to add some specific index for testing purpose.
+ /// Loads the segment with specified IndexLoadingConfig, honoring the
(possibly tier-scoped)
+ /// `skipSegmentPreprocess` flag. Callers that need to unconditionally
preprocess should use the explicit
+ /// `needPreprocess` overload instead.
public static ImmutableSegment load(File indexDir, IndexLoadingConfig
indexLoadingConfig,
@Nullable SegmentOperationsThrottlerSet segmentOperationsThrottlerSet,
@Nullable SegmentZKMetadata zkMetadata)
throws Exception {
- return load(indexDir, indexLoadingConfig, true,
segmentOperationsThrottlerSet, zkMetadata);
+ return load(indexDir, indexLoadingConfig,
!indexLoadingConfig.isSkipSegmentPreprocess(),
+ segmentOperationsThrottlerSet, zkMetadata);
Review Comment:
Considered using `ImmutableSegmentLoader.needPreprocess(segmentDirectory,
indexLoadingConfig)` here instead of only the `skipSegmentPreprocess` flag
check, but keeping the narrow check for these reasons:
- Cold-download path always deals with fresh segments. In most tables, the
configured indexes (inverted, range, bloom, startree, default columns, etc.)
are not built at segment-generation time — they're materialized during
preprocess. So `SegmentPreProcessor.create(...).needProcess()` will almost
always return `true` for a freshly downloaded segment when
`skipSegmentPreprocess=false`.
- Calling the full `needPreprocess(...)` opens the `SegmentDirectory` and
walks the handler chain just to compute that answer. That's extra I/O for the
same result the narrow check produces (`needPreprocess=true`).
- The one case where the full check would help — a fresh segment that
already ships with every configured index built — is rare on cold download.
So the narrow fix keeps behavior identical for the common case and avoids
paying an extra directory-open + handler-walk per cold load. Warm-load and
reload paths already invoke the full `needPreprocess(...)` in
`BaseTableDataManager`, which is where the smart check pays off.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
