Copilot commented on code in PR #18451:
URL: https://github.com/apache/pinot/pull/18451#discussion_r3214590959
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/DictionaryBasedIndexBuilder.java:
##########
@@ -141,18 +144,12 @@ public static void
addRawValuesViaDictionary(DictionaryBasedInvertedIndexCreator
} else {
for (int i = 0; i < numDocs; i++) {
byte[][] values = forwardIndexReader.getBytesMV(i, readerContext);
- int[] dictIds = new int[values.length];
- for (int j = 0; j < values.length; j++) {
- dictIds[j] = dictionary.indexOf(new ByteArray(values[j]));
- }
- creator.addBytesMV(values, dictIds);
+ creator.addBytesMV(values, lookupDictIds(dictionary, values));
}
}
return;
default:
- throw new IllegalStateException(
- "Unsupported data type for dictionary-based index over raw values:
" + columnMetadata.getDataType()
- + " (column: " + columnName + ")");
+ throw new IllegalStateException("Unsupported data type: " + dataType);
Review Comment:
The new default branch exception message drops useful context that
previously helped debug production failures (e.g., which column triggered the
unsupported type, and that this was during dictionary-based index building).
Consider restoring the column name and/or the operation context in the message
while still using the cached `dataType` variable.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/creator/ForwardIndexCreator.java:
##########
@@ -84,8 +84,7 @@ default void add(Object cellValue, int dictId) {
}
@Override
- default void add(Object[] cellValues, @Nullable int[] dictIds)
- throws IOException {
+ default void add(Object[] cellValues, @Nullable int[] dictIds) {
if (isDictionaryEncoded()) {
putDictIdMV(dictIds);
} else {
Review Comment:
ForwardIndexCreator is part of pinot-segment-spi (public SPI). Removing
`throws IOException` from these overridden `add*` method declarations is
source-incompatible for any external implementations that currently override
these methods with `throws IOException` (even though the bytecode signature is
unchanged). Consider keeping the `throws IOException` clauses for SPI
stability, or adding new non-throwing convenience methods instead of changing
existing method declarations.
--
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]
