deepthi912 commented on PR #19394: URL: https://github.com/apache/pinot/pull/19394#issuecomment-5505418725
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]
