somandal commented on code in PR #9740:
URL: https://github.com/apache/pinot/pull/9740#discussion_r1018627320
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -147,11 +187,48 @@ Map<String, Operation>
computeOperation(SegmentDirectory.Reader segmentReader)
}
}
+ // Get list of columns with forward index and those without forward index
+ Set<String> existingForwardIndexColumns =
+
segmentReader.toSegmentDirectory().getColumnsWithIndex(ColumnIndexType.FORWARD_INDEX);
+ Set<String> existingForwardIndexDisabledColumns = new HashSet<>();
+ for (String column : existingAllColumns) {
+ if (!existingForwardIndexColumns.contains(column)) {
+ existingForwardIndexDisabledColumns.add(column);
+ }
+ }
+
// From new column config.
Set<String> newNoDictColumns =
_indexLoadingConfig.getNoDictionaryColumns();
+ Set<String> newForwardIndexDisabledColumns =
_indexLoadingConfig.getForwardIndexDisabledColumns();
for (String column : existingAllColumns) {
- if (existingNoDictColumns.contains(column) &&
!newNoDictColumns.contains(column)) {
+ if (existingForwardIndexColumns.contains(column) &&
newForwardIndexDisabledColumns.contains(column)) {
+ ColumnMetadata columnMetadata =
_segmentMetadata.getColumnMetadataFor(column);
+ if (columnMetadata != null && columnMetadata.isSorted()) {
+ // Check if the column is sorted. If sorted, disabling forward index
should be a no-op. Do not return an
+ // operation for this column related to disabling forward index.
+ LOGGER.warn("Trying to disable the forward index for a sorted column
{}, ignoring", column);
+ continue;
+ }
+
+ // Existing column has a forward index. New column config disables the
forward index
+ if (existingDictColumns.contains(column)) {
+ columnOperationMap.put(column,
Operation.DISABLE_FORWARD_INDEX_FOR_DICT_COLUMN);
+ } else {
+ columnOperationMap.put(column,
+ Operation.DISABLE_FORWARD_INDEX_FOR_RAW_COLUMN);
+ }
+ } else if (existingForwardIndexDisabledColumns.contains(column)
+ && !newForwardIndexDisabledColumns.contains(column)) {
+ // TODO: Add support: existing column has its forward index disabled.
New column config enables the forward
+ // index
+ LOGGER.warn("Enabling forward index on a forward index disabled column
{} is not yet supported", column);
Review Comment:
done, modified to throw an exception
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -147,11 +187,48 @@ Map<String, Operation>
computeOperation(SegmentDirectory.Reader segmentReader)
}
}
+ // Get list of columns with forward index and those without forward index
+ Set<String> existingForwardIndexColumns =
+
segmentReader.toSegmentDirectory().getColumnsWithIndex(ColumnIndexType.FORWARD_INDEX);
+ Set<String> existingForwardIndexDisabledColumns = new HashSet<>();
+ for (String column : existingAllColumns) {
+ if (!existingForwardIndexColumns.contains(column)) {
+ existingForwardIndexDisabledColumns.add(column);
+ }
+ }
+
// From new column config.
Set<String> newNoDictColumns =
_indexLoadingConfig.getNoDictionaryColumns();
+ Set<String> newForwardIndexDisabledColumns =
_indexLoadingConfig.getForwardIndexDisabledColumns();
for (String column : existingAllColumns) {
- if (existingNoDictColumns.contains(column) &&
!newNoDictColumns.contains(column)) {
+ if (existingForwardIndexColumns.contains(column) &&
newForwardIndexDisabledColumns.contains(column)) {
+ ColumnMetadata columnMetadata =
_segmentMetadata.getColumnMetadataFor(column);
+ if (columnMetadata != null && columnMetadata.isSorted()) {
+ // Check if the column is sorted. If sorted, disabling forward index
should be a no-op. Do not return an
+ // operation for this column related to disabling forward index.
+ LOGGER.warn("Trying to disable the forward index for a sorted column
{}, ignoring", column);
+ continue;
+ }
+
+ // Existing column has a forward index. New column config disables the
forward index
+ if (existingDictColumns.contains(column)) {
+ columnOperationMap.put(column,
Operation.DISABLE_FORWARD_INDEX_FOR_DICT_COLUMN);
+ } else {
+ columnOperationMap.put(column,
+ Operation.DISABLE_FORWARD_INDEX_FOR_RAW_COLUMN);
+ }
+ } else if (existingForwardIndexDisabledColumns.contains(column)
+ && !newForwardIndexDisabledColumns.contains(column)) {
+ // TODO: Add support: existing column has its forward index disabled.
New column config enables the forward
+ // index
+ LOGGER.warn("Enabling forward index on a forward index disabled column
{} is not yet supported", column);
+ } else if (existingForwardIndexDisabledColumns.contains(column)
+ && newForwardIndexDisabledColumns.contains(column)) {
+ // Forward index is disabled for the existing column and should remain
disabled based on the latest config
+ Preconditions.checkState(existingDictColumns.contains(column) &&
!newNoDictColumns.contains(column),
+ String.format("Not allowed to disable the dictionary for forward
index disabled column %s", column));
Review Comment:
done
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
