siddharthteotia commented on code in PR #9740:
URL: https://github.com/apache/pinot/pull/9740#discussion_r1017673936
##########
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:
Here again the tableConfig will become misleading since we are silently
ignoring. May be we should throw error for now indicating backfill / refresh
until supported on reload path ?
--
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]
