You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "deepthi912 (via GitHub)" <gi...@apache.org> on 2026/08/29 01:00:23 UTC
[PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
deepthi912 opened a new pull request, #19394:
URL: https://github.com/apache/pinot/pull/19394
## Summary
`ImmutableSegmentLoader#needPreprocess` already short-circuits when `skipSegmentPreprocess` is set (and after #19391, honors the tier-scoped override too). Callers on the warm-load and reload paths route through `needPreprocess`, so they observe the flag correctly. But the cold-download path in `BaseTableDataManager` — `downloadAndLoadSegment` and `replaceSegmentIfCrcMismatch` — invokes the 4-arg `ImmutableSegmentLoader#load` overload that hard-codes `needPreprocess=true` and calls `preprocess` directly. 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 `preprocess` itself. Every caller now behaves consistently — whether preprocess is reached through `needPreprocess` or directly, `skipSegmentPreprocess=true` results in a uniform no-op.
## Changes
- `ImmutableSegmentLoader.preprocess`: early-return when `indexLoadingConfig.isSkipSegmentPreprocess()` is true. Log at INFO so the skip is auditable.
- `LoaderTest.testSkipSegmentPreprocessShortCircuitsPreprocess`: builds a V1 segment, invokes `preprocess` with `skipSegmentPreprocess=true` on an ILC that would otherwise trigger V1→V3 conversion, and asserts the segment stays V1 with no V3 directory materialized. Fails without the fix.
## Test plan
- [x] `./mvnw -pl pinot-segment-local test -Dtest=LoaderTest -Dcheckstyle.skip -Dsurefire.failIfNoSpecifiedTests=false` — 11/11 pass.
- [x] `./mvnw -pl pinot-segment-local checkstyle:check spotless:check` — clean.
## Related
- #19391 (merged) made the flag tier-aware. This PR closes the last runtime gap so the flag has effect on every code path — including fresh cold downloads.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #19394:
URL: https://github.com/apache/pinot/pull/19394#discussion_r3899548880
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -158,6 +158,16 @@ public static void preprocess(File indexDir, IndexLoadingConfig indexLoadingConf
Preconditions.checkArgument(indexDir.isDirectory(), "Index directory: %s does not exist or is not a directory",
indexDir);
+ // Respect skipSegmentPreprocess uniformly across every caller — including cold-download load paths
Review Comment:
I see your point, but needPreprocess would open segment directories and check segment versions as well which we don't need in cases of new segment generations and also open segment index handlers to check for preProcess when we can simply assume the worst case and just proceed with `preProcess`.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on PR #19394:
URL: https://github.com/apache/pinot/pull/19394#issuecomment-5505445655
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #19394:
URL: https://github.com/apache/pinot/pull/19394#discussion_r3911426735
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -158,6 +158,16 @@ public static void preprocess(File indexDir, IndexLoadingConfig indexLoadingConf
Preconditions.checkArgument(indexDir.isDirectory(), "Index directory: %s does not exist or is not a directory",
indexDir);
+ // Respect skipSegmentPreprocess uniformly across every caller — including cold-download load paths
Review Comment:
You're right, and I'll switch to that pattern. My earlier push-back was framed around minion-built / batch cold-download where the fresh segment still needs its indexes built — but the pauseless-realtime case (which is really what this fix targets) is the opposite: the committer already ran preprocess before uploading, so non-committer replicas downloading the tar get a fully preprocessed segment and `needPreprocess()` would correctly return `false` and save the whole preprocess call.
The check is per-load but cheap; the work it gates is expensive. Paying the check universally is the safer contract and matches how `BaseTableDataManager` warm-load / reload paths already do it.
I'll update the fix to route through `needPreprocess(segmentDirectory, indexLoadingConfig)` at the call site so the cold-download callers use the same contract as the warm paths.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #19394:
URL: https://github.com/apache/pinot/pull/19394#discussion_r3899548880
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -158,6 +158,16 @@ public static void preprocess(File indexDir, IndexLoadingConfig indexLoadingConf
Preconditions.checkArgument(indexDir.isDirectory(), "Index directory: %s does not exist or is not a directory",
indexDir);
+ // Respect skipSegmentPreprocess uniformly across every caller — including cold-download load paths
Review Comment:
I see your point, but needPreProcess would open segment directories and check segment versions as well which we don't need in cases of new segment generations and also open segment index handlers to check for preProcess when we can simply assume the worst case and do `preProcess`.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -158,6 +158,16 @@ public static void preprocess(File indexDir, IndexLoadingConfig indexLoadingConf
Preconditions.checkArgument(indexDir.isDirectory(), "Index directory: %s does not exist or is not a directory",
indexDir);
+ // Respect skipSegmentPreprocess uniformly across every caller — including cold-download load paths
Review Comment:
I see your point, but needPreprocess would open segment directories and check segment versions as well which we don't need in cases of new segment generations and also open segment index handlers to check for preProcess when we can simply assume the worst case and do `preProcess`.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #19394:
URL: https://github.com/apache/pinot/pull/19394#discussion_r3897475097
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -158,6 +158,16 @@ public static void preprocess(File indexDir, IndexLoadingConfig indexLoadingConf
Preconditions.checkArgument(indexDir.isDirectory(), "Index directory: %s does not exist or is not a directory",
indexDir);
+ // Respect skipSegmentPreprocess uniformly across every caller — including cold-download load paths
Review Comment:
This is probably not the correct fix. The contract should be always checking `needPreprocess()` before invoking `preprocess()`, and some code path might miss the check.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Gate preprocess() on needPreprocess() in cold-download and consuming segment callers [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19394:
URL: https://github.com/apache/pinot/pull/19394#issuecomment-5522289830
Documentation follow-up: https://github.com/pinot-contrib/pinot-docs/pull/1029 (merged).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #19394:
URL: https://github.com/apache/pinot/pull/19394#discussion_r3899548880
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -158,6 +158,16 @@ public static void preprocess(File indexDir, IndexLoadingConfig indexLoadingConf
Preconditions.checkArgument(indexDir.isDirectory(), "Index directory: %s does not exist or is not a directory",
indexDir);
+ // Respect skipSegmentPreprocess uniformly across every caller — including cold-download load paths
Review Comment:
I see your point, but needPreProcess would open segment directories and check segment versions as well which we don't need in cases of new segment generations. But the same flow is used by segments whose crc changes. Are you talking about just making sure we have right `needPreprocess` variable before invoking it via load or just using `ImmutableSegmentLoader.needPreprocess(segmentDirectory, indexLoadingConfig)` method for any case?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #19394:
URL: https://github.com/apache/pinot/pull/19394#discussion_r3899548880
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -158,6 +158,16 @@ public static void preprocess(File indexDir, IndexLoadingConfig indexLoadingConf
Preconditions.checkArgument(indexDir.isDirectory(), "Index directory: %s does not exist or is not a directory",
indexDir);
+ // Respect skipSegmentPreprocess uniformly across every caller — including cold-download load paths
Review Comment:
I see your point, but needPreprocess would open segment directories and and in cases of new segment generations it also create segment index handlers to check for preProcess when we can simply assume the worst case and just proceed with `preProcess` which has almost same effect.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #19394:
URL: https://github.com/apache/pinot/pull/19394#discussion_r3911426735
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -158,6 +158,16 @@ public static void preprocess(File indexDir, IndexLoadingConfig indexLoadingConf
Preconditions.checkArgument(indexDir.isDirectory(), "Index directory: %s does not exist or is not a directory",
indexDir);
+ // Respect skipSegmentPreprocess uniformly across every caller — including cold-download load paths
Review Comment:
You're right, and I'll switch to that pattern. My earlier push-back was framed around minion-built / batch cold-download where the fresh segment still needs its indexes built — but the pauseless-realtime case (which is really what this fix targets) is the opposite: the committer already ran preprocess before uploading, so non-committer replicas downloading the tar get a fully preprocessed segment and `needPreprocess()` would correctly return `false` and save the whole preprocess call.
The check is per-load but cheap; the work it gates is expensive. Paying the check universally is the safer contract and matches how `BaseTableDataManager` warm-load / reload paths already do it.
I'll update the fix to route through `needPreprocess(segmentDirectory, indexLoadingConfig)` at the call site so the cold-download callers use the same contract as the warm paths.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #19394:
URL: https://github.com/apache/pinot/pull/19394#discussion_r3899548880
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -158,6 +158,16 @@ public static void preprocess(File indexDir, IndexLoadingConfig indexLoadingConf
Preconditions.checkArgument(indexDir.isDirectory(), "Index directory: %s does not exist or is not a directory",
indexDir);
+ // Respect skipSegmentPreprocess uniformly across every caller — including cold-download load paths
Review Comment:
I see your point, but needPreProcess would open segment directories and check segment versions as well which we don't need in cases of new segment generations. Are you talking about just making sure we have right `needPreprocess` variable before invoking it via load?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Gate preprocess() on needPreprocess() in cold-download and consuming segment callers [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on PR #19394:
URL: https://github.com/apache/pinot/pull/19394#issuecomment-5520387115
Done
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #19394:
URL: https://github.com/apache/pinot/pull/19394#discussion_r3899548880
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -158,6 +158,16 @@ public static void preprocess(File indexDir, IndexLoadingConfig indexLoadingConf
Preconditions.checkArgument(indexDir.isDirectory(), "Index directory: %s does not exist or is not a directory",
indexDir);
+ // Respect skipSegmentPreprocess uniformly across every caller — including cold-download load paths
Review Comment:
I see your point, but needPreProcess would open segment directories and check segment versions as well which we don't need in cases of new segment generations. But the same flow is used by segments whose crc changes. Are you talking about just making sure we have right `needPreprocess` variable before invoking it via load or just using `ImmutableSegmentLoader.needPreprocess(segmentDirectory, indexLoadingConfig)` method for any case before preprocess?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #19394:
URL: https://github.com/apache/pinot/pull/19394#discussion_r3899548880
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -158,6 +158,16 @@ public static void preprocess(File indexDir, IndexLoadingConfig indexLoadingConf
Preconditions.checkArgument(indexDir.isDirectory(), "Index directory: %s does not exist or is not a directory",
indexDir);
+ // Respect skipSegmentPreprocess uniformly across every caller — including cold-download load paths
Review Comment:
I see your point, but needPreProcess would open segment directories and check segment versions as well which we don't need in cases of new segment generations. But the same flow is used by segments whose crc changes. Are you talking about just making sure we have right `needPreprocess` variable before invoking it via load or just using `ImmutableSegmentLoader.needPreprocess(segmentDirectory, indexLoadingConfig)` method?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Gate preprocess() on needPreprocess() in cold-download and consuming segment callers [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #19394:
URL: https://github.com/apache/pinot/pull/19394
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #19394:
URL: https://github.com/apache/pinot/pull/19394#discussion_r3899548880
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -158,6 +158,16 @@ public static void preprocess(File indexDir, IndexLoadingConfig indexLoadingConf
Preconditions.checkArgument(indexDir.isDirectory(), "Index directory: %s does not exist or is not a directory",
indexDir);
+ // Respect skipSegmentPreprocess uniformly across every caller — including cold-download load paths
Review Comment:
I see your point, but needPreprocess would open segment directories and and in cases of new segment generations it also create segment index handlers to check for preProcess when we can simply assume the worst case and just proceed with `preProcess` which has almost same effect.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Honor skipSegmentPreprocess in ImmutableSegmentLoader#preprocess [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19394:
URL: https://github.com/apache/pinot/pull/19394#issuecomment-5459550403
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19394?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 0.00%. Comparing base ([`bd69e76`](https://app.codecov.io/gh/apache/pinot/commit/bd69e76f14e042d39cba2d4902249143ad793927?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`77385d5`](https://app.codecov.io/gh/apache/pinot/commit/77385d5c907316d2afc676d0cb444433ad4db721?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (bd69e76) and HEAD (77385d5). Click for more details.
>
> <details><summary>HEAD has 18 uploads less than BASE</summary>
>
>| Flag | BASE (bd69e76) | HEAD (77385d5) |
>|------|------|------|
>|lane-a|2|1|
>|integration|4|2|
>|java-25|6|2|
>|integration1|2|0|
>|temurin|6|2|
>|lane-b|2|1|
>|unittests1|1|0|
>|unittests|2|0|
>|unittests2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19394 +/- ##
=============================================
- Coverage 67.57% 0.00% -67.58%
=============================================
Files 3486 3 -3483
Lines 224134 6 -224128
Branches 35376 0 -35376
=============================================
- Hits 151464 0 -151464
+ Misses 60652 6 -60646
+ Partials 12018 0 -12018
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19394/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19394/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-100.00%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19394/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19394/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19394/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-67.58%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19394/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-100.00%)` | :arrow_down: |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19394/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19394/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-67.58%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19394/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19394/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19394/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19394?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org