Copilot commented on code in PR #18751:
URL: https://github.com/apache/pinot/pull/18751#discussion_r3531612549
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java:
##########
@@ -4109,53 +4111,33 @@ public void testSkipIndexes(boolean
useMultiStageQueryEngine)
throws Exception {
setUseMultiStageQueryEngine(useMultiStageQueryEngine);
long numTotalDocs = getCountStarResult();
-
assertEquals(postQuery(TEST_UPDATED_RANGE_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(),
numTotalDocs);
+
assertEquals(postQuery(TEST_UPDATED_INVERTED_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(),
numTotalDocs);
- // Update table config to add range and inverted index, and trigger reload
+ // Update table config to add inverted index, and trigger reload
TableConfig tableConfig = createOfflineTableConfig();
IndexingConfig indexingConfig = tableConfig.getIndexingConfig();
- indexingConfig.setRangeIndexColumns(UPDATED_RANGE_INDEX_COLUMNS);
indexingConfig.setInvertedIndexColumns(UPDATED_INVERTED_INDEX_COLUMNS);
updateTableConfig(tableConfig);
- reloadAllSegments(TEST_UPDATED_RANGE_INDEX_QUERY, false, numTotalDocs);
-
assertEquals(postQuery(TEST_UPDATED_RANGE_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(),
0L);
+ reloadAllSegments(TEST_UPDATED_INVERTED_INDEX_QUERY, false, numTotalDocs);
- // Ensure inv index is operational
assertEquals(postQuery(TEST_UPDATED_INVERTED_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(),
0L);
- // disallow use of range index on DivActualElapsedTime, inverted should be
unaffected
- String skipIndexes = buildSkipIndexesOption("DivActualElapsedTime=range");
- assertEquals(postQuery(skipIndexes +
TEST_UPDATED_INVERTED_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(),
- 0L);
- assertEquals(postQuery(skipIndexes +
TEST_UPDATED_RANGE_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(),
- numTotalDocs);
-
- // disallow use of inverted index on DivActualElapsedTime, range should be
unaffected
- skipIndexes = buildSkipIndexesOption("DivActualElapsedTime=inverted");
+ // disallow use of inverted index on DivActualElapsedTime
+ String skipIndexes =
buildSkipIndexesOption("DivActualElapsedTime=inverted");
// Confirm that inverted index is not used
assertFalse(postQuery(skipIndexes + " EXPLAIN PLAN FOR " +
TEST_UPDATED_INVERTED_INDEX_QUERY).toString()
.contains("FILTER_INVERTED_INDEX"));
- // EQ predicate type allows for using range index if one exists, even if
inverted index is skipped. That is why
- // we still see no docs scanned even though we skip the inverted index.
This is a good test to show that using
- // the skipIndexes can allow fine-grained experimentation of index usage
at query time.
- assertEquals(postQuery(skipIndexes +
TEST_UPDATED_INVERTED_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(),
- 0L);
- assertEquals(postQuery(skipIndexes +
TEST_UPDATED_RANGE_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(), 0L);
-
- // disallow use of both range and inverted indexes on
DivActualElapsedTime, neither should be used at query time
- skipIndexes =
buildSkipIndexesOption("DivActualElapsedTime=inverted,range");
assertEquals(postQuery(skipIndexes +
TEST_UPDATED_INVERTED_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(),
numTotalDocs);
- assertEquals(postQuery(skipIndexes +
TEST_UPDATED_RANGE_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(),
- numTotalDocs);
- // Update table config to remove the new indexes, and check if the new
indexes are removed
+ // Update table config to remove the new index, and check if the new index
is removed
updateTableConfig(_tableConfig);
reloadAllSegments(TEST_UPDATED_RANGE_INDEX_QUERY, false, numTotalDocs);
assertEquals(postQuery(TEST_UPDATED_RANGE_INDEX_QUERY).get("numEntriesScannedInFilter").asLong(),
numTotalDocs);
Review Comment:
`testSkipIndexes` removes the newly-added inverted index, but then calls
`reloadAllSegments` and asserts using `TEST_UPDATED_RANGE_INDEX_QUERY`. That
doesn’t actually verify the inverted index was removed, and can mask
regressions (or become flaky if a range index on that column is ever
introduced). Use the inverted-index query for the removal/reload assertions as
well.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1711,6 +1705,12 @@ private static void
validateIndexingConfigAndFieldConfigList(TableConfig tableCo
FieldSpec fieldSpec = schema.getFieldSpecFor(column);
Preconditions.checkState(fieldSpec != null, "Failed to find sorted
column: %s in schema", column);
Preconditions.checkState(fieldSpec.isSingleValueField(), "Cannot sort
on multi-value column: %s", column);
+ FieldIndexConfigs indexConfigsForSortedColumn =
indexConfigsMap.get(column);
+ for (IndexType<?, ?, ?> indexType : List.of(StandardIndexes.inverted(),
+ StandardIndexes.range())) {
+
Preconditions.checkState(indexConfigsForSortedColumn.getConfig(indexType).isDisabled(),
+ "Redundant to enable %s on a sorted column: %s",
indexType.getPrettyName(), fieldSpec.getName());
Review Comment:
This adds a new hard validation that rejects enabling inverted/range indexes
on sorted columns. There isn’t corresponding unit-test coverage asserting that
the config is rejected (and that the error message is stable). Adding a focused
test would prevent future regressions in this compatibility rule.
--
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]
