adding additional checks to improve index combatibility validation#18751
Open
RajaVinoth96 wants to merge 7 commits into
Open
adding additional checks to improve index combatibility validation#18751RajaVinoth96 wants to merge 7 commits into
RajaVinoth96 wants to merge 7 commits into
Conversation
RajaVinoth96
commented
Jun 12, 2026
| tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME) | ||
| .setNoDictionaryColumns(List.of("myCol2")) | ||
| .setInvertedIndexColumns(List.of("myCol1")) | ||
| .setSortedColumn("myCol1") |
Author
There was a problem hiding this comment.
Removed this because it is redundant to have an inverted index on a sorted column
RajaVinoth96
commented
Jun 12, 2026
| assertValid(tc); | ||
| } | ||
|
|
||
| @Test |
Author
There was a problem hiding this comment.
Dictionary encoding on json index is in itself an anti pattern
RajaVinoth96
commented
Jun 12, 2026
|
|
||
| @Test | ||
| public void testTextIndexStringColumnDictPasses() { | ||
| FieldConfig fc = new FieldConfig(STR_COL, EncodingType.DICTIONARY, IndexType.TEXT, null, null); |
Author
There was a problem hiding this comment.
Dictionary encoding on text index is an anti pattern
RajaVinoth96
commented
Jun 12, 2026
|
|
||
| @Test | ||
| public void testTextIndexNonStringColumnFails() { | ||
| FieldConfig fc = new FieldConfig(INT_COL, EncodingType.DICTIONARY, IndexType.TEXT, null, null); |
Author
There was a problem hiding this comment.
Dictionary encoding on text index is an anti pattern
RajaVinoth96
commented
Jun 12, 2026
| public void testRawWithExplicitDictAndInvertedPlusFstPasses() { | ||
| ObjectNode indexes = JsonUtils.newObjectNode(); | ||
| indexes.set("dictionary", JsonUtils.newObjectNode()); | ||
| FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW, null, List.of(IndexType.FST), |
Author
There was a problem hiding this comment.
Having both FST and inverted enabled on the same column is redundant.
added 2 commits
June 12, 2026 21:07
Contributor
There was a problem hiding this comment.
Pull request overview
This PR strengthens table/index configuration validation in pinot-segment-local by adding additional index-combination compatibility checks (and adjusting/removing some tests accordingly), plus a number of formatting-only cleanups.
Changes:
- Add new “redundant/anti-pattern” index combination checks (e.g., sorted-column vs other indexes; vector/text/range/json/etc vs other indexes).
- Update and remove certain unit tests in
IndexCombinationValidationTest/TableConfigUtilsTestto reflect the new validation expectations. - Apply assorted formatting/Javadoc/whitespace changes across several index type implementations.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java | Removes a sorted-column setting in one test case. |
| pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/IndexCombinationValidationTest.java | Removes/adjusts some index-combination validation tests. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java | Adds new validation for “redundant indexes on sorted columns” + formatting updates. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/vector/VectorIndexType.java | Adds validation preventing vector index coexisting with several other indexes. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/text/TextIndexType.java | Adds validation preventing text index coexisting with several other indexes. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/range/RangeIndexType.java | Adds validation preventing range index coexisting with several other indexes. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/json/JsonIndexType.java | Adds validation for JSON index (including new multi-value restriction) + additional anti-pattern checks. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/inverted/InvertedIndexType.java | Adds validation preventing inverted index coexisting with several other indexes. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/h3/H3IndexType.java | Adds validation preventing H3 index coexisting with several other indexes + formatting changes. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/fst/IFSTIndexType.java | Adds validation preventing IFST index coexisting with several other indexes. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/fst/FstIndexType.java | Adds validation preventing FST index coexisting with several other indexes. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/dictionary/DictionaryIndexType.java | Adds validation preventing dictionary index coexisting with several other indexes. |
| pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/bloom/BloomIndexType.java | Adds validation preventing bloom filter coexisting with several other indexes. |
| .setSortedColumn("myCol1") | ||
| .build(); | ||
| try { | ||
| // Enable forward index disabled flag for a column with inverted index and is sorted |
Comment on lines
84
to
+88
| DataType storedType = fieldSpec.getDataType().getStoredType(); | ||
| Preconditions.checkState(storedType == DataType.STRING || storedType == DataType.MAP, | ||
| "Cannot create JSON index on column: %s of stored type other than STRING or MAP", column); | ||
| Preconditions.checkState(fieldSpec.isSingleValueField(), "Cannot create JSON index on multi-value column: %s", | ||
| column); |
Comment on lines
+119
to
+123
| for (IndexType indexType : List.of( | ||
| StandardIndexes.vector(), | ||
| StandardIndexes.json(), | ||
| StandardIndexes.text(), | ||
| StandardIndexes.h3())) { |
Comment on lines
+89
to
+102
| for (IndexType indexType : List.of( | ||
| StandardIndexes.bloomFilter(), | ||
| StandardIndexes.dictionary(), | ||
| StandardIndexes.inverted(), | ||
| StandardIndexes.vector(), | ||
| StandardIndexes.range(), | ||
| StandardIndexes.json(), | ||
| StandardIndexes.fst(), | ||
| StandardIndexes.h3(), | ||
| StandardIndexes.ifst())) { | ||
| Preconditions.checkState(indexConfigs.getConfig(indexType).isDisabled(), | ||
| "Anti pattern to enable both text index and %s on column: %s", | ||
| indexType.getPrettyName(), fieldSpec.getName()); | ||
| } |
Comment on lines
+99
to
+107
| for (IndexType indexType : List.of( | ||
| StandardIndexes.bloomFilter(), | ||
| StandardIndexes.inverted(), | ||
| StandardIndexes.vector(), | ||
| StandardIndexes.json(), | ||
| StandardIndexes.text(), | ||
| StandardIndexes.fst(), | ||
| StandardIndexes.h3(), | ||
| StandardIndexes.ifst())) { |
Comment on lines
+1672
to
+1676
| for (IndexType indexType : List.of(StandardIndexes.bloomFilter(), StandardIndexes.inverted(), | ||
| StandardIndexes.range())) { | ||
| Preconditions.checkState(indexConfigsForSortedColumn.getConfig(indexType).isDisabled(), | ||
| "Redundant to enable %s on a sorted column: %s", indexType.getPrettyName(), fieldSpec.getName()); | ||
| } |
Comment on lines
136
to
138
| public IndexHandler createIndexHandler(SegmentDirectory segmentDirectory, Map<String, FieldIndexConfigs> configsByCol, | ||
| Schema schema, TableConfig tableConfig) { | ||
| Schema schema, TableConfig tableConfig) { | ||
| return new H3IndexHandler(segmentDirectory, configsByCol, tableConfig, schema); |
Comment on lines
171
to
173
| protected H3IndexReader createIndexReader(PinotDataBuffer dataBuffer, ColumnMetadata metadata, | ||
| H3IndexConfig indexConfig) { | ||
| H3IndexConfig indexConfig) { | ||
| return new ImmutableH3IndexReader(dataBuffer); |
Comment on lines
+90
to
+98
| for (IndexType indexType : List.of( | ||
| StandardIndexes.bloomFilter(), | ||
| StandardIndexes.vector(), | ||
| StandardIndexes.range(), | ||
| StandardIndexes.json(), | ||
| StandardIndexes.text(), | ||
| StandardIndexes.fst(), | ||
| StandardIndexes.h3(), | ||
| StandardIndexes.ifst())) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The PR adds additional check to enforce better index type compatibility checks.
Some of test cases have been removed or altered because the index configuration is either redundant or an anti pattern. Comments added to explain why it is the case.
Index_compatibility_matrix.xlsx