siddharthteotia commented on code in PR #9333:
URL: https://github.com/apache/pinot/pull/9333#discussion_r988647749
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -897,6 +903,40 @@ private static void validateFieldConfigList(@Nullable
List<FieldConfig> fieldCon
}
}
+ /**
+ * Validates the compatibility of the indexes if the column has the forward
index disabled. Throws exceptions due to
+ * compatibility mismatch. The checks performed are:
+ * - Validate dictionary is enabled.
+ * - Validate inverted index is enabled.
+ * - Validate that either no range index exists for column or the range
index version is at least 2 and isn't a
+ * multi-value column (since mulit-value defaults to index v1).
+ */
+ private static void validateForwardIndexDisabledIndexCompatibility(String
columnName, FieldConfig fieldConfig,
Review Comment:
(nit) we should consider keeping the check either here or during segment
generation in `SegmentColumnarIndexCreator`. For now it is fine to safeguard
multiple places until feature gets used for a while.
I am guessing basic validation at the table config level will always be
needed since it will come handy even during enabling / disabling the feature
on an existing column. So an unsupported combination will be caught when user
attempts to update the table config in controller as opposed to detecting later
on in the reload path or segment refresh / push path
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -897,6 +903,40 @@ private static void validateFieldConfigList(@Nullable
List<FieldConfig> fieldCon
}
}
+ /**
+ * Validates the compatibility of the indexes if the column has the forward
index disabled. Throws exceptions due to
+ * compatibility mismatch. The checks performed are:
+ * - Validate dictionary is enabled.
+ * - Validate inverted index is enabled.
+ * - Validate that either no range index exists for column or the range
index version is at least 2 and isn't a
+ * multi-value column (since mulit-value defaults to index v1).
+ */
+ private static void validateForwardIndexDisabledIndexCompatibility(String
columnName, FieldConfig fieldConfig,
Review Comment:
(nit) we should consider keeping the check either here or during segment
generation in `SegmentColumnarIndexCreator`. For now it is fine to safeguard
multiple places until feature gets used for a while.
I am guessing basic validation at the table config level will always be
needed since it will come handy even during enabling / disabling the feature
on an existing column. So an unsupported combination will be caught when user
attempts to update the table config in controller as opposed to detecting later
on in the reload path and/or segment refresh / push 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]
