Copilot commented on code in PR #18229:
URL: https://github.com/apache/pinot/pull/18229#discussion_r3336965198
##########
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:
The comment refers to a top-level `FieldConfig.codecSpec`, but `codecSpec`
is only supported under `indexes.forward.codecSpec` (there is no top-level
field). This is misleading for future readers of the test.
##########
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:
This regression comment also mentions a non-existent top-level
`FieldConfig.codecSpec`. The scenario being tested is `encodingType` resolution
(e.g. via `noDictionaryColumns`) combined with `indexes.forward.codecSpec`, so
the wording should reflect the actual config path.
##########
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:
Builder copy constructor assigns `_configs = other._configs`, which aliases
the same mutable map between the original `ForwardIndexConfig` and the copied
config. Since `getConfigs()` exposes the map, mutating one instance can
unexpectedly mutate the other. Copy the map to avoid shared mutable state
across config instances.
--
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]
