Copilot commented on code in PR #17269:
URL: https://github.com/apache/pinot/pull/17269#discussion_r2558384415
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -74,7 +74,11 @@ public static ForwardIndexConfig getDefault() {
}
public static ForwardIndexConfig getDisabled() {
- return new ForwardIndexConfig(true, null, null, null, null, null, null,
null, null);
+ return new ForwardIndexConfig(true, null, null, null, null, null, null);
Review Comment:
The disabled config constructor call is missing the `rawEncoding` parameter
(should be 7 arguments instead of 7). This will cause a compilation error since
the private constructor at line 100 expects 8 parameters.
```suggestion
return new ForwardIndexConfig(true, null, null, null, null, null, null,
false);
```
##########
pinot-core/src/test/java/org/apache/pinot/queries/ForwardIndexHandlerReloadQueriesTest.java:
##########
@@ -621,40 +661,41 @@ public void testRangeIndexAfterReload()
validateBeforeAfterQueryResults(resultRows1, resultRows2);
}
+ @Test
+ public void testRawForwardColumnIndexAddAndRemove()
+ throws Exception {
+ String filterValue = escapeSingleQuotes(getMostFrequentColumn5Value());
+ String filterQuery = String.format("SELECT COUNT(*) FROM %s WHERE column5
= '%s'", RAW_TABLE_NAME, filterValue);
+
+ BrokerResponseNative baselineResponse = getBrokerResponse(filterQuery);
+ assertTrue(baselineResponse.getExceptions() == null ||
baselineResponse.getExceptions().size() == 0);
+ ResultTable baselineResultTable = baselineResponse.getResultTable();
+ assertNotNull(baselineResultTable);
+ List<Object[]> baselineRows = baselineResultTable.getRows();
+ long baselineEntriesScanned =
baselineResponse.getNumEntriesScannedInFilter();
+ assertTrue(baselineEntriesScanned > 0);
+ }
+
/**
* As a part of segmentReload, the ForwardIndexHandler will perform the
following operations:
*
* column1 -> change compression.
* column6 -> disable dictionary
- * column9 -> disable dictionary
+ * column9 -> keep dictionary (range index)
Review Comment:
Comment is outdated. The test code at line 693 shows rangeIndexColumns as an
empty list, so column9 doesn't have a range index anymore. Update the comment
to reflect actual behavior.
```suggestion
* column9 -> keep dictionary
```
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/DictionaryIndexConfig.java:
##########
@@ -114,4 +117,41 @@ public String toString() {
return "DictionaryIndexConfig{" + "\"disabled\": true}";
}
}
+
+ /**
+ * Returns {@code true} if a dictionary must be created for the given column
based on the configured indexes.
+ */
+ public static boolean isDictionaryRequired(FieldSpec fieldSpec,
FieldIndexConfigs fieldIndexConfigs) {
+ if (fieldIndexConfigs.getConfig(StandardIndexes.dictionary()).isEnabled())
{
Review Comment:
Potential NullPointerException if `getConfig(StandardIndexes.dictionary())`
returns null. Add null check before calling `isEnabled()`.
```suggestion
IndexConfig dictionaryConfig =
fieldIndexConfigs.getConfig(StandardIndexes.dictionary());
if (dictionaryConfig != null && dictionaryConfig.isEnabled()) {
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -303,7 +303,13 @@ private boolean
createDictionaryForColumn(ColumnIndexCreationInfo info, SegmentG
String column = spec.getName();
FieldIndexConfigs fieldIndexConfigs =
config.getIndexConfigsByColName().get(column);
+ // Infer dictionary existence based on index
+ boolean dictionaryRequired =
DictionaryIndexConfig.isDictionaryRequired(spec, fieldIndexConfigs);
if
(fieldIndexConfigs.getConfig(StandardIndexes.dictionary()).isDisabled()) {
+ if (dictionaryRequired) {
+ LOGGER.warn("Found indexes {} requires dictionary, but dictionary is
disabled explicitly.",
Review Comment:
Grammatical error: 'requires' should be 'require' to agree with the plural
subject 'indexes'.
```suggestion
LOGGER.warn("Found indexes {} require dictionary, but dictionary is
disabled explicitly.",
```
--
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]
