Copilot commented on code in PR #17269:
URL: https://github.com/apache/pinot/pull/17269#discussion_r2655075285
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java:
##########
@@ -581,13 +584,17 @@ public void testDisableDictAndOtherIndexesSV()
long oldRangeIndexSize =
new
SegmentMetadataImpl(INDEX_DIR).getColumnMetadataFor(COLUMN10_NAME).getIndexSizeFor(StandardIndexes.range());
_noDictionaryColumns.add(COLUMN10_NAME);
- checkForwardIndexCreation(COLUMN10_NAME, 3960, 12, _schema, false, false,
false, 0, ChunkCompressionType.LZ4, true,
- 0, DataType.INT, 100000);
- validateIndex(StandardIndexes.range(), COLUMN10_NAME, 3960, 12, false,
false, false, 0, true, 0,
- ChunkCompressionType.LZ4, false, DataType.INT, 100000);
- long newRangeIndexSize =
- new
SegmentMetadataImpl(INDEX_DIR).getColumnMetadataFor(COLUMN10_NAME).getIndexSizeFor(StandardIndexes.range());
- assertNotEquals(oldRangeIndexSize, newRangeIndexSize);
+ try {
+ checkForwardIndexCreation(COLUMN10_NAME, 3960, 12, _schema, false,
false, false, 0, ChunkCompressionType.LZ4,
+ true, 0, DataType.INT, 100000);
+ validateIndex(StandardIndexes.range(), COLUMN10_NAME, 3960, 12, false,
false, false, 0, true, 0,
+ ChunkCompressionType.LZ4, false, DataType.INT, 100000);
+ long newRangeIndexSize = new
SegmentMetadataImpl(INDEX_DIR).getColumnMetadataFor(COLUMN10_NAME)
+ .getIndexSizeFor(StandardIndexes.range());
+ assertNotEquals(oldRangeIndexSize, newRangeIndexSize);
+ } catch (RuntimeException e) {
+ // Forward-index-disabled columns cannot rebuild range index without an
existing forward index; accept failure.
+ }
Review Comment:
The catch block swallows the RuntimeException without any assertion. If this
failure is expected, add an assertion to verify it's the expected exception
type or message, otherwise the test might pass for the wrong reason.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -61,6 +64,28 @@ public void init(SegmentGeneratorConfig segmentCreationSpec,
SegmentIndexCreatio
segmentCreationSpec.getIndexConfigsByColName().size());
initializeCommon(segmentCreationSpec, segmentIndexCreationInfo,
indexCreationInfoMap,
schema, outDir, colIndexes, immutableToMutableIdMap, instanceType);
+
+ // Although NullValueVector is implemented as an index, it needs to be
treated in a different way than other indexes
+ // Process all field specs (not just indexed columns) for null value
vectors
+ for (FieldSpec fieldSpec : schema.getAllFieldSpecs()) {
+ String columnName = fieldSpec.getName();
+ // Create null value vector for non-indexed nullable columns
+ if (!_colIndexes.containsKey(columnName)) {
+ if (isNullable(fieldSpec, schema, segmentCreationSpec)) {
+ // Initialize Null value vector map
+ LOGGER.info("Column: {} is nullable", columnName);
+ _colIndexes.put(columnName,
+ new ColumnIndexCreators(columnName, fieldSpec, null, List.of(),
+ new NullValueVectorCreator(outDir, columnName)));
+ } else {
+ LOGGER.info("Column: {} is not nullable", columnName);
+ }
+ }
+ }
+ }
+
+ private boolean isNullable(FieldSpec fieldSpec, Schema schema,
SegmentGeneratorConfig config) {
+ return schema.isEnableColumnBasedNullHandling() ? fieldSpec.isNullable() :
config.isDefaultNullHandlingEnabled();
}
Review Comment:
This method duplicates null-handling logic that likely exists elsewhere in
the codebase. Consider extracting this to a shared utility class to avoid
duplication and ensure consistent null-handling behavior.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1345,6 +1349,67 @@ private static void
validateMultiColumnTextIndex(MultiColumnTextIndexConfig mult
}
}
+ private static void validateExplicitDictionaryForRawForwardIndex(TableConfig
tableConfig,
+ Map<String, FieldIndexConfigs> indexConfigsMap) {
+ IndexingConfig indexingConfig = tableConfig.getIndexingConfig();
+ Set<String> noDictionaryColumns = new HashSet<>();
+ if (indexingConfig.getNoDictionaryColumns() != null) {
+ noDictionaryColumns.addAll(indexingConfig.getNoDictionaryColumns());
+ }
+ if (indexingConfig.getNoDictionaryConfig() != null) {
+
noDictionaryColumns.addAll(indexingConfig.getNoDictionaryConfig().keySet());
+ }
+
+ Map<String, FieldConfig> fieldConfigMap = new HashMap<>();
+ List<FieldConfig> fieldConfigs = tableConfig.getFieldConfigList();
+ if (fieldConfigs != null) {
+ for (FieldConfig fieldConfig : fieldConfigs) {
+ fieldConfigMap.put(fieldConfig.getName(), fieldConfig);
+ }
+ }
+
+ for (Map.Entry<String, FieldIndexConfigs> entry :
indexConfigsMap.entrySet()) {
+ String column = entry.getKey();
+ FieldIndexConfigs indexConfigs = entry.getValue();
+ ForwardIndexConfig forwardIndexConfig =
indexConfigs.getConfig(StandardIndexes.forward());
+ IndexConfig invertedIndexConfig =
indexConfigs.getConfig(StandardIndexes.inverted());
+ if (!invertedIndexConfig.isEnabled()) {
Review Comment:
The code checks `invertedIndexConfig.isEnabled()` without first checking if
`invertedIndexConfig` is null. This could result in a NullPointerException. Add
a null check before calling `isEnabled()`.
```suggestion
if (invertedIndexConfig == null || !invertedIndexConfig.isEnabled()) {
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentIndexCreationDriverImpl.java:
##########
@@ -506,7 +506,7 @@ void collectStatsAndIndexCreationInfo()
DataType storedType = fieldSpec.getDataType().getStoredType();
ColumnStatistics columnProfile =
_segmentStats.getColumnProfileFor(column);
DictionaryIndexConfig dictionaryIndexConfig =
indexConfigsMap.get(column).getConfig(StandardIndexes.dictionary());
- boolean createDictionary = dictionaryIndexConfig.isDisabled();
+ boolean createDictionary = dictionaryIndexConfig.isEnabled();
Review Comment:
The variable name `createDictionary` conflicts with its negated usage
pattern. The original code checked `isDisabled()` which was inverted logic. Now
checking `isEnabled()` makes the variable name correct, but verify all
downstream usages expect this boolean to be true when dictionary should be
created.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java:
##########
@@ -596,10 +603,14 @@ public void testDisableDictAndOtherIndexesSV()
_fieldConfigMap.put(EXISTING_STRING_COL_DICT,
new FieldConfig(EXISTING_STRING_COL_DICT,
FieldConfig.EncodingType.RAW, List.of(FieldConfig.IndexType.TEXT),
null, null));
- checkForwardIndexCreation(EXISTING_STRING_COL_DICT, 9, 4, _schema, false,
false, false, 0, ChunkCompressionType.LZ4,
- true, 0, DataType.STRING, 100000);
- validateIndex(StandardIndexes.forward(), EXISTING_STRING_COL_DICT, 9, 4,
false, false, false, 0, true, 0, null,
- false, DataType.STRING, 100000);
+ try {
+ checkForwardIndexCreation(EXISTING_STRING_COL_DICT, 9, 4, _schema,
false, false, false, 0,
+ ChunkCompressionType.LZ4, true, 0, DataType.STRING, 100000);
+ validateIndex(StandardIndexes.forward(), EXISTING_STRING_COL_DICT, 9, 4,
false, false, false, 0, true, 0,
+ null, false, DataType.STRING, 100000);
+ } catch (RuntimeException e) {
+ // Forward index not available for disabled dictionary; skip validation
in this case.
+ }
Review Comment:
The catch block swallows the RuntimeException without any assertion. Add
validation to ensure the caught exception is the expected one, otherwise the
test could pass for unintended reasons.
##########
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,
false);
Review Comment:
The `getDisabled()` method constructs a ForwardIndexConfig with 8
parameters, but the private constructor at line 100 expects 8 parameters
including `rawEncoding` as the last one. However, line 77 only passes 8
arguments where the 8th is `false` (rawEncoding). The issue is that this call
should use the 7-parameter public constructor or the call signature doesn't
match the intended constructor.
```suggestion
return new ForwardIndexConfig(true, null, null, null, null, null, null);
```
--
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]
