Jackie-Jiang commented on code in PR #10260:
URL: https://github.com/apache/pinot/pull/10260#discussion_r1115123268
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/DefaultIndexCreatorProvider.java:
##########
@@ -76,6 +75,10 @@ public ForwardIndexCreator
newForwardIndexCreator(IndexCreationContext.Forward c
throws Exception {
if (!context.hasDictionary()) {
// Dictionary disabled columns
+ if (context.forwardIndexDisabled()) {
Review Comment:
(minor) Suggest moving the check to the caller side to be consistent with
other index creators. Basically when invoking this method, forward index should
be created
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -93,12 +94,10 @@ public class ForwardIndexHandler extends BaseIndexHandler {
private final Schema _schema;
protected enum Operation {
Review Comment:
(optional) Ideally we can remove the `Operation` enum, and update the index
in steps. Can be handled separately
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -608,119 +568,125 @@ public void indexRow(GenericRow row)
columnValueToIndex = FieldConfig.TEXT_INDEX_DEFAULT_RAW_VALUE;
}
}
- switch (forwardIndexCreator.getValueType()) {
- case INT:
- forwardIndexCreator.putInt((int) columnValueToIndex);
- break;
- case LONG:
- forwardIndexCreator.putLong((long) columnValueToIndex);
- break;
- case FLOAT:
- forwardIndexCreator.putFloat((float) columnValueToIndex);
- break;
- case DOUBLE:
- forwardIndexCreator.putDouble((double) columnValueToIndex);
- break;
- case BIG_DECIMAL:
- forwardIndexCreator.putBigDecimal((BigDecimal)
columnValueToIndex);
- break;
- case STRING:
- forwardIndexCreator.putString((String) columnValueToIndex);
- break;
- case BYTES:
- forwardIndexCreator.putBytes((byte[]) columnValueToIndex);
- break;
- case JSON:
- if (columnValueToIndex instanceof String) {
+ if (forwardIndexCreator != null) {
+ switch (forwardIndexCreator.getValueType()) {
+ case INT:
+ forwardIndexCreator.putInt((int) columnValueToIndex);
+ break;
+ case LONG:
+ forwardIndexCreator.putLong((long) columnValueToIndex);
+ break;
+ case FLOAT:
+ forwardIndexCreator.putFloat((float) columnValueToIndex);
+ break;
+ case DOUBLE:
+ forwardIndexCreator.putDouble((double) columnValueToIndex);
+ break;
+ case BIG_DECIMAL:
+ forwardIndexCreator.putBigDecimal((BigDecimal)
columnValueToIndex);
+ break;
+ case STRING:
forwardIndexCreator.putString((String) columnValueToIndex);
- } else if (columnValueToIndex instanceof byte[]) {
+ break;
+ case BYTES:
forwardIndexCreator.putBytes((byte[]) columnValueToIndex);
- }
- break;
- default:
- throw new IllegalStateException();
+ break;
+ case JSON:
+ if (columnValueToIndex instanceof String) {
+ forwardIndexCreator.putString((String) columnValueToIndex);
+ } else if (columnValueToIndex instanceof byte[]) {
+ forwardIndexCreator.putBytes((byte[]) columnValueToIndex);
+ }
+ break;
+ default:
+ throw new IllegalStateException();
+ }
}
}
} else {
if (dictionaryCreator != null) {
//dictionary encoded
int[] dictIds = dictionaryCreator.indexOfMV(columnValueToIndex);
- forwardIndexCreator.putDictIdMV(dictIds);
+ if (forwardIndexCreator != null) {
+ forwardIndexCreator.putDictIdMV(dictIds);
+ }
DictionaryBasedInvertedIndexCreator invertedIndexCreator =
_invertedIndexCreatorMap.get(columnName);
if (invertedIndexCreator != null) {
invertedIndexCreator.add(dictIds, dictIds.length);
}
} else {
// for text index on raw columns, check the config to determine if
actual raw value should
// be stored or not
- if (textIndexCreator != null &&
!shouldStoreRawValueForTextIndex(columnName)) {
- Object value =
_columnProperties.get(columnName).get(FieldConfig.TEXT_INDEX_RAW_VALUE);
- if (value == null) {
- value = FieldConfig.TEXT_INDEX_DEFAULT_RAW_VALUE;
- }
- if (forwardIndexCreator.getValueType().getStoredType() ==
DataType.STRING) {
- columnValueToIndex = new String[]{String.valueOf(value)};
- } else if (forwardIndexCreator.getValueType().getStoredType() ==
DataType.BYTES) {
- columnValueToIndex = new
byte[][]{String.valueOf(value).getBytes(UTF_8)};
- } else {
- throw new RuntimeException("Text Index is only supported for
STRING and BYTES stored type");
- }
- }
- Object[] values = (Object[]) columnValueToIndex;
- int length = values.length;
- switch (forwardIndexCreator.getValueType()) {
- case INT:
- int[] ints = new int[length];
- for (int i = 0; i < length; i++) {
- ints[i] = (Integer) values[i];
- }
- forwardIndexCreator.putIntMV(ints);
- break;
- case LONG:
- long[] longs = new long[length];
- for (int i = 0; i < length; i++) {
- longs[i] = (Long) values[i];
+ if (forwardIndexCreator != null) {
+ if (textIndexCreator != null &&
!shouldStoreRawValueForTextIndex(columnName)) {
Review Comment:
(minor) Merge these 2 if conditions
```suggestion
if (forwardIndexCreator != null && textIndexCreator != null &&
!shouldStoreRawValueForTextIndex(columnName)) {
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -110,86 +109,81 @@ public ForwardIndexHandler(SegmentDirectory
segmentDirectory, IndexLoadingConfig
@Override
public boolean needUpdateIndices(SegmentDirectory.Reader segmentReader)
throws Exception {
- Map<String, Operation> columnOperationMap =
computeOperation(segmentReader);
- return !columnOperationMap.isEmpty();
+ Map<String, List<Operation>> columnOperationsMap =
computeOperations(segmentReader);
+ return !columnOperationsMap.isEmpty();
}
@Override
public void updateIndices(SegmentDirectory.Writer segmentWriter,
IndexCreatorProvider indexCreatorProvider)
throws Exception {
- Map<String, Operation> columnOperationMap =
computeOperation(segmentWriter);
- if (columnOperationMap.isEmpty()) {
+ Map<String, List<Operation>> columnOperationsMap =
computeOperations(segmentWriter);
+ if (columnOperationsMap.isEmpty()) {
return;
}
- for (Map.Entry<String, Operation> entry : columnOperationMap.entrySet()) {
+ for (Map.Entry<String, List<Operation>> entry :
columnOperationsMap.entrySet()) {
String column = entry.getKey();
- Operation operation = entry.getValue();
-
- switch (operation) {
- case DISABLE_FORWARD_INDEX_FOR_DICT_COLUMN: {
- // Deletion of the forward index will be handled outside the index
handler to ensure that other index
- // handlers that need the forward index to construct their own
indexes will have it available.
- // The existing forward index must be in dictionary format for this
to be a no-op.
- _tmpForwardIndexColumns.add(column);
- break;
- }
- case DISABLE_FORWARD_INDEX_FOR_RAW_COLUMN: {
- // The forward index has been disabled for a column which has a
noDictionary based forward index. A dictionary
- // and inverted index need to be created before we can delete the
forward index. We create a dictionary here,
- // but let the InvertedIndexHandler handle the creation of the
inverted index. We create a temporary
- // forward index here which is dictionary based and allow the post
deletion step handle the actual deletion
- // of the forward index.
- createDictBasedForwardIndex(column, segmentWriter,
indexCreatorProvider);
- if (!segmentWriter.hasIndexFor(column,
ColumnIndexType.FORWARD_INDEX)) {
- throw new IOException(String.format("Temporary forward index was
not created for column: %s", column));
- }
- _tmpForwardIndexColumns.add(column);
- break;
- }
- case ENABLE_FORWARD_INDEX_FOR_DICT_COLUMN: {
- createForwardIndexIfNeeded(segmentWriter, column,
indexCreatorProvider, false);
- if (!segmentWriter.hasIndexFor(column, ColumnIndexType.DICTIONARY)) {
- throw new IOException(
- String.format("Dictionary should still exist after rebuilding
forward index for dictionary column: %s",
- column));
- }
- break;
- }
- case ENABLE_FORWARD_INDEX_FOR_RAW_COLUMN: {
- createForwardIndexIfNeeded(segmentWriter, column,
indexCreatorProvider, false);
- if (segmentWriter.hasIndexFor(column, ColumnIndexType.DICTIONARY)) {
- throw new IOException(
- String.format("Dictionary should not exist after rebuilding
forward index for raw column: %s", column));
- }
- break;
+ List<Operation> operations = entry.getValue();
+
+ for (Operation operation : operations) {
+ switch (operation) {
+ case DISABLE_FORWARD_INDEX:
+ // Deletion of the forward index will be handled outside the index
handler to ensure that other index
+ // handlers that need the forward index to construct their own
indexes will have it available.
+ _tmpForwardIndexColumns.add(column);
+ break;
+ case ENABLE_FORWARD_INDEX:
+ ColumnMetadata columnMetadata =
createForwardIndexIfNeeded(segmentWriter, column, indexCreatorProvider,
+ false);
+ if (columnMetadata.hasDictionary()) {
+ if (!segmentWriter.hasIndexFor(column,
ColumnIndexType.DICTIONARY)) {
+ throw new IOException(String.format("Dictionary should still
exist after rebuilding forward index "
Review Comment:
Not introduced in this PR, but suggest changing it to
`IllegalStateException` since it is not really `IOException`. Same for other
places where the operation is illegal
--
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]
