You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2026/03/23 22:55:48 UTC
[PR] Always create inverted index during segment creation [pinot]
Jackie-Jiang opened a new pull request, #17951:
URL: https://github.com/apache/pinot/pull/17951
This is discussed in Slack `pinot-dev` and `pinot-committers` channel.
Making inverted index behavior consistent with other index types, where index is generated during segment creation, instead of segment loading. This greatly simplifies the management overhead, and also reduce the chance of bugs.
# Behavior Change
When configured, inverted index is always created during segment creation. `createInvertedIndexDuringSegmentGeneration` within `IndexingConfig` is no longer honored
--
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] Always create inverted index during segment creation [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17951:
URL: https://github.com/apache/pinot/pull/17951#discussion_r2978446338
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/SegmentGeneratorConfig.java:
##########
@@ -68,7 +65,6 @@ public enum TimeColumnType {
EPOCH, SIMPLE_DATE
}
Review Comment:
This change removes the previously public constant for the deprecated custom property key (generate.inverted.index.before.push). If external tooling still references the constant, this becomes a compilation break even though the setting is deprecated. Consider keeping the constant as `@Deprecated` (even if it is ignored) for compatibility, or documenting the removal as an intentional breaking change.
```suggestion
/**
* @deprecated This custom property is ignored by current segment generation logic and is kept
* only for backward compatibility with external tools that may reference it.
*/
@Deprecated
public static final String GENERATE_INVERTED_INDEX_BEFORE_PUSH = "generate.inverted.index.before.push";
```
--
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] Always create inverted index during segment creation [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17951:
URL: https://github.com/apache/pinot/pull/17951#discussion_r2978432109
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/SegmentGeneratorConfig.java:
##########
@@ -137,10 +133,6 @@ public enum TimeColumnType {
* This will not work once we start supporting multiple time columns (DateTimeFieldSpec)
Review Comment:
The constructor Javadoc mentions falling back to the schema if the table config is null, but the constructor now enforces non-null via Preconditions.checkNotNull(tableConfig). Please update the Javadoc to match the current behavior so it doesn’t mislead callers.
--
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] Always create inverted index during segment creation [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17951:
URL: https://github.com/apache/pinot/pull/17951#issuecomment-4114421455
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17951?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 `0%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.49%. Comparing base ([`884bcf4`](https://app.codecov.io/gh/apache/pinot/commit/884bcf4d3ce70b8eeb5dc44e8ce10540afab7617?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a3bd27c`](https://app.codecov.io/gh/apache/pinot/commit/a3bd27c0859a13d14dcfdce6c253989ba86c8b71?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/17951?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...l/realtime/converter/RealtimeSegmentConverter.java](https://app.codecov.io/gh/apache/pinot/pull/17951?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fconverter%2FRealtimeSegmentConverter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9jb252ZXJ0ZXIvUmVhbHRpbWVTZWdtZW50Q29udmVydGVyLmphdmE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17951?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 (884bcf4) and HEAD (a3bd27c). Click for more details.
>
> <details><summary>HEAD has 8 uploads less than BASE</summary>
>
>| Flag | BASE (884bcf4) | HEAD (a3bd27c) |
>|------|------|------|
>|java-21|2|1|
>|unittests1|2|1|
>|unittests|3|1|
>|temurin|3|1|
>|java-11|1|0|
>|unittests2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17951 +/- ##
============================================
- Coverage 63.18% 55.49% -7.69%
+ Complexity 1484 728 -756
============================================
Files 3194 2497 -697
Lines 193257 142255 -51002
Branches 29711 22786 -6925
============================================
- Hits 122115 78951 -43164
+ Misses 61558 56590 -4968
+ Partials 9584 6714 -2870
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17951/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/17951/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/17951/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.49% <0.00%> (-7.66%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17951/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.49% <0.00%> (-7.69%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17951/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.49% <0.00%> (-7.69%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17951/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.49% <0.00%> (-0.03%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17951/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 Sentry](https://app.codecov.io/gh/apache/pinot/pull/17951?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
Re: [PR] Always create inverted index during segment creation [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #17951:
URL: https://github.com/apache/pinot/pull/17951#issuecomment-4136252086
📚 Documentation PR opened: https://github.com/pinot-contrib/pinot-docs/pull/578
--
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] Always create inverted index during segment creation [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #17951:
URL: https://github.com/apache/pinot/pull/17951
--
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] Always create inverted index during segment creation [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17951:
URL: https://github.com/apache/pinot/pull/17951#discussion_r2978092666
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/IndexingConfig.java:
##########
@@ -96,9 +96,12 @@ public class IndexingConfig extends BaseJsonConfig {
// TODO: Add a new configuration related to the segment generation
private boolean _autoGeneratedInvertedIndex;
- private boolean _createInvertedIndexDuringSegmentGeneration;
private String _segmentNameGeneratorType;
+ /// @deprecated Keep it for backward compatibility during upgrade. Remove it after 1.5.0 release
Review Comment:
The deprecated `createInvertedIndexDuringSegmentGeneration` flag is kept for backward compatibility, but the new deprecation note doesn’t state that the flag is now ignored. Please update the comment/Javadoc to explicitly say it has no effect (and ideally point to the new behavior) to avoid misleading config authors.
```suggestion
/// @deprecated This flag is ignored and has no effect. Inverted index creation during segment generation
/// is now controlled by the standard indexing configuration (for example, `invertedIndexColumns`) and
/// segment generation settings. Kept only for backward compatibility during upgrade; safe to remove
/// after the 1.5.0 release.
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableConfigBuilder.java:
##########
@@ -514,7 +507,6 @@ public TableConfig build() {
indexingConfig.setSortedColumn(Collections.singletonList(_sortedColumn));
}
indexingConfig.setInvertedIndexColumns(_invertedIndexColumns);
- indexingConfig.setCreateInvertedIndexDuringSegmentGeneration(_createInvertedIndexDuringSegmentGeneration);
indexingConfig.setNoDictionaryColumns(_noDictionaryColumns);
indexingConfig.setOnHeapDictionaryColumns(_onHeapDictionaryColumns);
indexingConfig.setBloomFilterColumns(_bloomFilterColumns);
Review Comment:
`TableConfigBuilder` previously exposed `setCreateInvertedIndexDuringSegmentGeneration(...)` but this PR removes it entirely, which is a compile-time breaking change for any external code constructing `TableConfig` via this builder. Consider keeping the method (and the backing field) as `@Deprecated` (optionally a no-op) for at least one release, matching the backward-compat approach taken in `IndexingConfig`.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/SegmentGeneratorConfig.java:
##########
@@ -197,28 +189,6 @@ public SegmentGeneratorConfig(TableConfig tableConfig, Schema schema, boolean cr
}
_indexConfigsByColName = FieldIndexConfigsUtil.createIndexConfigsByColName(tableConfig, schema);
-
- // NOTE: By default inverted indexes are not created during segment creation
- // There are 2 ways to configure creating inverted index during segment generation:
- // - Set 'generate.inverted.index.before.push' to 'true' in custom config (deprecated)
- // - Enable 'createInvertedIndexDuringSegmentGeneration' in indexing config
- // TODO: Clean up the table configs with the deprecated settings, and always use the one in the indexing config
- // TODO 2: Decide what to do with this. Index-spi is based on the idea that TableConfig is the source of truth
- List<String> invertedIndexColumns = indexingConfig.getInvertedIndexColumns();
- if (!createInvertedIndex && CollectionUtils.isNotEmpty(invertedIndexColumns)) {
- Map<String, String> customConfigs = tableConfig.getCustomConfig().getCustomConfigs();
- boolean customConfigEnabled =
- customConfigs != null && Boolean.parseBoolean(customConfigs.get(GENERATE_INV_BEFORE_PUSH_DEPREC_PROP));
- boolean indexingConfigEnable = indexingConfig.isCreateInvertedIndexDuringSegmentGeneration();
- if (!customConfigEnabled && !indexingConfigEnable) {
- //noinspection rawtypes
- IndexType inverted = StandardIndexes.inverted();
- for (String column : invertedIndexColumns) {
- _indexConfigsByColName.computeIfPresent(column,
- (k, v) -> new FieldIndexConfigs.Builder(v).undeclare(inverted).build());
- }
- }
- }
}
Review Comment:
This PR changes a major behavior (inverted indexes are now always declared/generated during segment creation), but there doesn’t appear to be a unit test asserting the new behavior or backward-compat behavior (e.g., the deprecated flag/custom configs no longer suppress generation). Please add/extend tests (e.g., in `SegmentGeneratorConfigTest` or a segment generation test) to lock this in.
--
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