xiangfu0 commented on code in PR #18229:
URL: https://github.com/apache/pinot/pull/18229#discussion_r3338717582
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -302,8 +399,10 @@ public Builder(ForwardIndexConfig other, EncodingType
encodingType) {
_disabled = other.isDisabled();
_encodingType = Preconditions.checkNotNull(encodingType, "encodingType
must not be null");
_compressionCodec = other._compressionCodec;
+ _codecSpec = other._codecSpec;
_deriveNumDocsPerChunk = other._deriveNumDocsPerChunk;
_rawIndexWriterVersion = other._rawIndexWriterVersion;
+ _rawIndexWriterVersionExplicit = other._rawIndexWriterVersion !=
_defaultRawIndexWriterVersion;
_targetMaxChunkSize = other._targetMaxChunkSize;
_targetDocsPerChunk = other._targetDocsPerChunk;
_configs = other._configs;
Review Comment:
Fixed — the Builder copy constructor now does a defensive `new
HashMap<>(other._configs)` so the copied and original configs no longer share a
mutable map.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1555,6 +1555,95 @@ public void testValidateFieldConfig() {
} catch (Exception e) {
fail("Should pass since inverted index has explicit dictionary config,
but got: " + e.getMessage());
}
+
+ // codecSpec inside the modern indexes.forward block should be validated
the same as top-level
+ // FieldConfig.codecSpec.
Review Comment:
Updated the comment to reflect that codecSpec is configured only under
`indexes.forward` (no top-level field).
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1555,6 +1555,95 @@ public void testValidateFieldConfig() {
} catch (Exception e) {
fail("Should pass since inverted index has explicit dictionary config,
but got: " + e.getMessage());
}
+
+ // codecSpec inside the modern indexes.forward block should be validated
the same as top-level
+ // FieldConfig.codecSpec.
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+ {
+ ObjectNode indexes = JsonUtils.newObjectNode();
+ ObjectNode forward = JsonUtils.newObjectNode();
+ forward.put("codecSpec", "CODEC(DELTA,ZSTD(3))");
+ indexes.set("forward", forward);
+ FieldConfig nestedCodecSpecConfig =
+ new FieldConfig("intCol", FieldConfig.EncodingType.RAW, null,
Collections.emptyList(), null, null, indexes,
+ null, null);
+ tableConfig.setFieldConfigList(Arrays.asList(nestedCodecSpecConfig));
+ TableConfigUtils.validate(tableConfig, schema);
+ }
+
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+ FieldConfig stringCompressionCodecSpec =
rawFieldConfigWithCodecSpec("myCol1", "SNAPPY");
+ FieldConfig mvCompressionCodecSpec = rawFieldConfigWithCodecSpec("myCol2",
"LZ4");
+ tableConfig.setFieldConfigList(Arrays.asList(stringCompressionCodecSpec,
mvCompressionCodecSpec));
+ TableConfigUtils.validate(tableConfig, schema);
+
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+ try {
+ FieldConfig mvTransformCodecSpec = rawFieldConfigWithCodecSpec("myCol2",
"CODEC(DELTA,LZ4)");
+ tableConfig.setFieldConfigList(Arrays.asList(mvTransformCodecSpec));
+ TableConfigUtils.validate(tableConfig, schema);
+ fail("Should fail for transform codecSpec on multi-value column");
+ } catch (Exception e) {
+ assertTrue(e.getMessage().contains("only supports single-value columns")
+ && e.getMessage().contains("myCol2"),
+ "Unexpected error: " + e.getMessage());
+ }
+
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+ try {
+ ObjectNode indexes = JsonUtils.newObjectNode();
+ ObjectNode forward = JsonUtils.newObjectNode();
+ forward.put("codecSpec", "CODEC(DELTA,UNKNOWN)");
+ indexes.set("forward", forward);
+ FieldConfig nestedCodecSpecConfig =
+ new FieldConfig("intCol", FieldConfig.EncodingType.RAW, null,
Collections.emptyList(), null, null, indexes,
+ null, null);
+ tableConfig.setFieldConfigList(Arrays.asList(nestedCodecSpecConfig));
+ TableConfigUtils.validate(tableConfig, schema);
+ fail("Should fail for unknown codec inside indexes.forward.codecSpec");
+ } catch (Exception e) {
+ assertTrue(e.getMessage().contains("Unknown codec"), "Unexpected error:
" + e.getMessage());
+ }
+
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+ try {
+ ObjectNode indexes = JsonUtils.newObjectNode();
+ ObjectNode forward = JsonUtils.newObjectNode();
+ forward.put("codecSpec", "LZ4");
+ indexes.set("forward", forward);
+ FieldConfig nestedCodecSpecConfig =
+ new FieldConfig("intCol", FieldConfig.EncodingType.DICTIONARY, null,
Collections.emptyList(), null, null,
+ indexes, null, null);
+ tableConfig.setFieldConfigList(Arrays.asList(nestedCodecSpecConfig));
+ TableConfigUtils.validate(tableConfig, schema);
+ fail("Should fail for codecSpec on dictionary-encoded forward index");
+ } catch (Exception e) {
+ assertEquals(e.getMessage(), "codecSpec requires RAW forward-index
encoding for column: intCol");
+ }
+
+ // Regression: codecSpec validation now runs via `IndexType.validate(...)`
after
+ // `FieldIndexConfigsUtil` resolves overrides — not by the prior early
raw-FieldConfig pre-pass.
+ // A column listed in `noDictionaryColumns` with
FieldConfig.encodingType=RAW + codecSpec must
+ // pass validation.
Review Comment:
Reworded — the scenario is RAW resolved via `noDictionaryColumns` combined
with `indexes.forward.codecSpec`.
--
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]
