Jackie-Jiang commented on code in PR #19284:
URL: https://github.com/apache/pinot/pull/19284#discussion_r3834188728
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java:
##########
@@ -204,9 +204,15 @@ protected ColumnConfigDeserializer<ForwardIndexConfig>
createDeserializer() {
// Pop processed columns so the post-loop scan only emits defaults
for columns with no FieldConfig at all.
boolean inNoDictionaryList = noDictionaryColumns.remove(column);
- // `forwardIndexDisabled` short-circuits everything else.
+ JsonNode forwardIndexNode =
fieldConfig.getIndexes().get(INDEX_DISPLAY_NAME);
+
+ // `forwardIndexDisabled` short-circuits everything else. Do not let
that legacy flag silently discard a
+ // codecSpec from the modern forward-index config.
Map<String, String> properties = fieldConfig.getProperties();
if (properties != null && isDisabled(properties)) {
+ JsonNode codecSpecNode = forwardIndexNode == null ? null :
forwardIndexNode.get("codecSpec");
+ Preconditions.checkState(codecSpecNode == null ||
codecSpecNode.isNull(),
+ "codecSpec cannot be configured when the forward index is
disabled for column: %s", column);
Review Comment:
（minor) We can just keep the existing logic. There is no check to any other
field as well
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -110,7 +114,16 @@ private ForwardIndexConfig(@JsonProperty("disabled")
@Nullable Boolean disabled,
// implicit behavior where ForwardIndexConfig had no encoding
distinction). Programmatic callers must use
// Builder(EncodingType) and pass an explicit value, typically from
FieldConfig.getEncodingType().
_encodingType = encodingType == null ? EncodingType.DICTIONARY :
encodingType;
- _compressionCodec = getActualCompressionCodec(compressionCodec,
chunkCompressionType, dictIdCompressionType);
+ CompressionCodec actualCompressionCodec =
+ getActualCompressionCodec(compressionCodec, chunkCompressionType,
dictIdCompressionType);
Review Comment:
(minor) Not introduced in this PR, but please annotate the return as
`@Nullable`
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexTypeTest.java:
##########
@@ -302,6 +302,79 @@ public void newConfigDefault()
assertEquals(ForwardIndexConfig.getDefault(FieldConfig.EncodingType.DICTIONARY));
}
+ @Test
+ public void newRawConfigWithCodecSpecInheritsFieldEncoding()
Review Comment:
(minor) Suggest reverting this in this PR
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -110,7 +114,16 @@ private ForwardIndexConfig(@JsonProperty("disabled")
@Nullable Boolean disabled,
// implicit behavior where ForwardIndexConfig had no encoding
distinction). Programmatic callers must use
// Builder(EncodingType) and pass an explicit value, typically from
FieldConfig.getEncodingType().
_encodingType = encodingType == null ? EncodingType.DICTIONARY :
encodingType;
- _compressionCodec = getActualCompressionCodec(compressionCodec,
chunkCompressionType, dictIdCompressionType);
+ CompressionCodec actualCompressionCodec =
+ getActualCompressionCodec(compressionCodec, chunkCompressionType,
dictIdCompressionType);
Review Comment:
(minor) You can keep the assignment as is, and do the check on
`_compressionCodec`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java:
##########
@@ -242,8 +247,14 @@ protected ColumnConfigDeserializer<ForwardIndexConfig>
createDeserializer() {
+ "but indexes.forward.compressionCodec=%s", column,
fcCodec, inner);
}
+ JsonNode innerCodecSpecNode = forwardIndexNode.get("codecSpec");
Review Comment:
(minor) Suggest reverting the changes in this file. We can modify it when
wiring the new config in
--
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]
