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/06/15 07:06:35 UTC
[PR] Add skipSecondaryIndexes option to skip building secondary indexes [pinot]
deepthi912 opened a new pull request, #18761:
URL: https://github.com/apache/pinot/pull/18761
## Summary
Adds a new table-level config `tableIndexConfig.skipSecondaryIndexes` (default `false`). When enabled:
1. **New segments are built with only the forward index.** `BaseSegmentCreator` skips every non-forward index creator during the per-row build loop and short-circuits `updatePostSegmentCreationIndexes` (so star-tree is also skipped). Dictionary is still written for dict-encoded columns because the forward index needs it to resolve dict-ids.
2. **Existing segments load as-is and preprocess does not add/remove secondary indexes.** `SegmentPreProcessor.process()` runs only the `ForwardIndexHandler` (still allows forward-encoding upgrades) and default-column materialization; it skips the loop over all other index handlers, plus star-tree and multi-col text. Min/max generation is also skipped — it is purely a query-time pruning hint. `needProcess()` mirrors the same skips so preprocess isn't triggered just to add a secondary index. The on-disk loader (`PhysicalColumnIndexContainer`) is intentionally untouched, so segments with pre-existing secondary indexes keep serving them.
## Motivation
Useful for workloads that don't need secondary indexes during initial ingest (or for an async "build indexes later" flow where the flag is later flipped off and a reload runs the missing handlers).
## Config
```json
{
"tableIndexConfig": {
"skipSecondaryIndexes": true
}
}
```
## Test plan
- [ ] Unit test: build a segment with the flag on; verify only `<col>.fwd` (+ `<col>.dict` for dict-encoded columns) is present.
- [ ] Unit test: load an existing segment with secondary indexes under the flag; verify on-disk indexes still serve queries.
- [ ] Unit test: with the flag on, add a default column to schema and trigger reload; verify the default column is materialized but no secondary indexes are built.
- [ ] Unit test: flip flag off and reload; verify previously-skipped indexes get built.
- [ ] Integration test: star-tree config + flag on → star-tree not built; flip off → star-tree built on next reload.
--
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] Add skipSecondaryIndexes option to skip building secondary indexes [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 commented on code in PR #18761:
URL: https://github.com/apache/pinot/pull/18761#discussion_r3438182962
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -1043,6 +1051,12 @@ protected void reloadSegments(List<SegmentDataManager> segmentDataManagers, Inde
protected void reloadSegment(SegmentDataManager segmentDataManager, IndexLoadingConfig indexLoadingConfig,
boolean forceDownload)
throws Exception {
+ // Reload path — always run full preprocess against the latest table config, regardless of the persisted
+ // tableIndexConfig.skipSegmentPreprocess. The override lives on this transient IndexLoadingConfig only and
+ // is discarded when the reload finishes. Centralizing it here means every public reload entry point
+ // (reloadSegment / reloadAllSegments / reloadSegments) gets the reload semantics for free; boot /
+ // state-transition loads continue to use a plain fetchIndexLoadingConfig() and honor skipSegmentPreprocess.
+ indexLoadingConfig.setOverrideSkipSegmentPreprocess(true);
Review Comment:
Yes, will going to do that! Ack
--
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] Add skipSecondaryIndexes option to skip building secondary indexes [pinot]
Posted by "deepthi912 (via GitHub)" <gi...@apache.org>.
deepthi912 closed pull request #18761: Add skipSecondaryIndexes option to skip building secondary indexes
URL: https://github.com/apache/pinot/pull/18761
--
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] Add skipSecondaryIndexes option to skip building secondary indexes [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18761:
URL: https://github.com/apache/pinot/pull/18761#discussion_r3435689413
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -1043,6 +1051,12 @@ protected void reloadSegments(List<SegmentDataManager> segmentDataManagers, Inde
protected void reloadSegment(SegmentDataManager segmentDataManager, IndexLoadingConfig indexLoadingConfig,
boolean forceDownload)
throws Exception {
+ // Reload path — always run full preprocess against the latest table config, regardless of the persisted
+ // tableIndexConfig.skipSegmentPreprocess. The override lives on this transient IndexLoadingConfig only and
+ // is discarded when the reload finishes. Centralizing it here means every public reload entry point
+ // (reloadSegment / reloadAllSegments / reloadSegments) gets the reload semantics for free; boot /
+ // state-transition loads continue to use a plain fetchIndexLoadingConfig() and honor skipSegmentPreprocess.
+ indexLoadingConfig.setOverrideSkipSegmentPreprocess(true);
Review Comment:
This changes the contract of the existing `tableIndexConfig.skipSegmentPreprocess` flag for every reload path, not just the async index-materialization flow. Today a table that sets this flag can rely on reloads honoring the persisted forward-only load policy; after this override, any manual/controller reload will silently build all secondary indexes instead. That is a behavior change for an existing config with real CPU/disk blast radius for current deployments. Please gate the bypass behind a new explicit reload API/flag instead of unconditionally overriding the persisted setting here.
--
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] Add skipSecondaryIndexes option to skip building secondary indexes [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18761:
URL: https://github.com/apache/pinot/pull/18761#issuecomment-4705828929
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18761?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `32.43243%` with `25 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.95%. Comparing base ([`5617ee7`](https://app.codecov.io/gh/apache/pinot/commit/5617ee70f4e90a6e9daa6617a264172c37387651?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`43ec9f5`](https://app.codecov.io/gh/apache/pinot/commit/43ec9f51e5eed920a23517a87799164a9378ba23?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18761?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ocal/segment/index/loader/SegmentPreProcessor.java](https://app.codecov.io/gh/apache/pinot/pull/18761?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Findex%2Floader%2FSegmentPreProcessor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2xvYWRlci9TZWdtZW50UHJlUHJvY2Vzc29yLmphdmE=) | 28.00% | [14 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18761?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...local/segment/creator/impl/BaseSegmentCreator.java](https://app.codecov.io/gh/apache/pinot/pull/18761?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Fcreator%2Fimpl%2FBaseSegmentCreator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9CYXNlU2VnbWVudENyZWF0b3IuamF2YQ==) | 25.00% | [2 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18761?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...local/segment/index/loader/IndexLoadingConfig.java](https://app.codecov.io/gh/apache/pinot/pull/18761?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Findex%2Floader%2FIndexLoadingConfig.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2xvYWRlci9JbmRleExvYWRpbmdDb25maWcuamF2YQ==) | 0.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18761?src=pr&el=tree&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 (5617ee7) and HEAD (43ec9f5). Click for more details.
>
> <details><summary>HEAD has 16 uploads less than BASE</summary>
>
>| Flag | BASE (5617ee7) | HEAD (43ec9f5) |
>|------|------|------|
>|java-21|5|1|
>|unittests|2|1|
>|temurin|5|1|
>|unittests2|1|0|
>|integration|3|0|
>|integration1|1|0|
>|integration2|1|0|
>|custom-integration1|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18761 +/- ##
============================================
- Coverage 64.76% 56.95% -7.82%
+ Complexity 1309 1 -1308
============================================
Files 3380 2591 -789
Lines 209573 151107 -58466
Branches 32805 24449 -8356
============================================
- Hits 135735 86062 -49673
+ Misses 62914 57739 -5175
+ Partials 10924 7306 -3618
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18761/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18761/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18761/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/18761/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/18761/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18761/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.95% <32.43%> (-7.82%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18761/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.95% <32.43%> (-7.82%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18761/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.95% <32.43%> (-7.82%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18761/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.95% <32.43%> (-0.01%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18761/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/18761?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