somandal commented on code in PR #9740:
URL: https://github.com/apache/pinot/pull/9740#discussion_r1015939773
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -147,11 +190,45 @@ 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 != null
+ && newForwardIndexDisabledColumns.contains(column)) {
+ // Existing column has a forward index. New column config disables the
forward index
+ Preconditions.checkState(!newNoDictColumns.contains(column),
+ String.format("Must enable dictionary for disabling the forward
index on column: %s", column));
+
Preconditions.checkState(_indexLoadingConfig.getInvertedIndexColumns().contains(column),
+ String.format("Must enable inverted index for disabling the
forward index on column: %s", column));
+ if (existingDictColumns.contains(column)) {
+ columnOperationMap.put(column, Operation.DELETE_FORWARD_INDEX);
+ } else {
+ columnOperationMap.put(column,
+
Operation.CREATE_TEMP_DICTIONARY_BASED_FORWARD_INDEX_FROM_EXISTING_RAW_FORWARD_INDEX);
+ }
+ } else if (existingForwardIndexDisabledColumns.contains(column) &&
newForwardIndexDisabledColumns != null
+ && !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 != null
+ && 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));
+ } else if (existingNoDictColumns.contains(column) &&
!newNoDictColumns.contains(column)) {
Review Comment:
as discussed offline, modified the enum ordering instead. wanted to get the
checks for forward index disabled to get handled earlier. This is especially
necessary as in the future we intend to enable forward index for forward index
disabled columns and other operations will always have to check for whether
forward index is disabled before performing their operations.
--
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]
