xiangfu0 commented on code in PR #19308:
URL: https://github.com/apache/pinot/pull/19308#discussion_r4110298024
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/CompressionStatsMetadata.java:
##########
@@ -47,10 +47,12 @@ private CompressionStatsMetadata(@Nullable Long
forwardIndexUncompressedValueSiz
_dictionaryEncodedUncompressedValueSizeInBytes =
dictionaryUncompressedValueSizeInBytes;
}
- /// Creates metadata for a raw forward index, or unavailable metadata when
either required value is absent.
+ /// Creates metadata for a raw forward index, or unavailable metadata when
the size was not tracked. The
+ /// compression type is nullable because codec-pipeline V7 indexes do not
have one legacy
+ /// [ChunkCompressionType]; their uncompressed size is still valid and must
remain reportable.
public static CompressionStatsMetadata forRawForwardIndex(long
uncompressedValueSizeInBytes,
@Nullable ChunkCompressionType chunkCompressionType) {
- return uncompressedValueSizeInBytes >= 0 && chunkCompressionType != null
+ return uncompressedValueSizeInBytes >= 0
? new CompressionStatsMetadata(uncompressedValueSizeInBytes,
chunkCompressionType, null) : UNAVAILABLE;
Review Comment:
This relaxation also changes what the reload paths persist for V7 columns
(`ForwardIndexHandler.rewriteForwardIndex`, `rewriteDictToRawForwardIndex`, and
`createDictionaryForRawForwardIndex`, which carries the existing raw stats
forward), but only segment creation is covered
(`SegmentCompressionStatsReaderTest.testV7SegmentCreationPersistsAndReportsCompressionStats`).
I couldn't find the V7 reload-with-stats tests the description mentions.
Verified locally with a throwaway test at this head
(`compressionStatsEnabled=true`), all passing:
1. V7 column at creation: size = `numDocs × 8`, type null.
2. Legacy `SNAPPY` → `DELTA,ZSTD(3)`: size kept, type null.
3. `DELTA,LZ4` → `T64,LZ4` → legacy `LZ4`: size kept; type null, then `LZ4`.
4. Dictionary → RAW with `codecSpec`: raw size persisted, dictionary size
cleared.
5. Adding a dictionary to a V7 raw column: raw stats carried over, type
stays null.
6. V7 rewrite with stats disabled: keys cleared.
Worth adding these as cases in `ForwardIndexHandlerCompressionStatsTest`.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/FieldConfig.java:
##########
@@ -148,23 +148,45 @@ public enum IndexType {
public enum CompressionCodec {
//@formatter:off
+ /// No compression. This is the default for `METRIC` columns and has no
`codecSpec` equivalent:
+ /// the DSL has no identity codec and rejects a blank spec, so this
remains the only way to state
+ /// "uncompressed" explicitly.
PASS_THROUGH(true, false),
+ /// Snappy compression for raw forward indexes. Prefer
`codecSpec="SNAPPY"` in new configs;
+ /// existing `compressionCodec` uses remain supported.
SNAPPY(true, false),
+ /// Zstandard compression for raw forward indexes. Prefer
`codecSpec="ZSTD(3)"` in new configs;
+ /// existing `compressionCodec` uses remain supported.
ZSTANDARD(true, false),
+ /// LZ4 compression for raw forward indexes. Prefer `codecSpec="LZ4"` in
new configs;
+ /// existing `compressionCodec` uses remain supported.
LZ4(true, false),
+ /// GZIP (DEFLATE) compression for raw forward indexes. Prefer
`codecSpec="GZIP"` in new configs;
+ /// existing `compressionCodec` uses remain supported.
Review Comment:
**Should fix before merge.** These four constants recommend `codecSpec`
unconditionally, but `codecSpec` is narrower than `compressionCodec`:
- `ForwardIndexType.validateCodecPipelineShape` accepts it only on
single-value INT/LONG raw columns, so following this advice on a STRING, BYTES,
BIG_DECIMAL, FLOAT, DOUBLE or multi-value column fails table-config validation.
- On INT/LONG it switches the column to the V7 format, which pre-V7 servers
cannot read; rolling back needs the rewrite-and-re-push steps documented on
`ForwardIndexConfig.getCodecSpec()`.
The #19309 design doc says other column shapes "continue to use legacy
`compressionCodec`". Suggest scoping the advice, e.g.:
```java
/// Snappy compression for raw forward indexes. For single-value INT/LONG
raw columns on a cluster where
/// every server reads V7, `codecSpec="SNAPPY"` is the codec-pipeline
equivalent; other column shapes keep
/// using this value.
```
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1798,6 +1798,145 @@ public void testValidateFieldConfig() {
}
}
+ /// Semantic table-config-time validation of `codecSpec` (via
`ForwardIndexType.validate`):
+ /// well-formed specs on supported column shapes are accepted, and malformed
or type-incompatible
+ /// specs fail with precise errors.
+ @Test
+ public void testCodecSpecValidation() {
Review Comment:
Non-blocking: most of this duplicates `testCodecSpecTableConfigValidation`
just below (unknown codec, MV `LZ4`, STRING `SNAPPY`, disabled forward index +
`codecSpec`, and the positive `ZSTD(3)` case), and
`rawFieldConfigWithCodecSpec` duplicates `fieldConfigWithCodecSpec(column, RAW,
spec)`. Consider folding only the new cases (`T64,DELTA,LZ4` ordering,
`DELTA,T64,LZ4`, `DELTA,DELTADELTA,LZ4`, LONG `DELTADELTA,LZ4`, MV `DELTA,LZ4`)
into the existing test using `assertCodecSpecValidationFails`.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1798,6 +1798,145 @@ public void testValidateFieldConfig() {
}
}
+ /// Semantic table-config-time validation of `codecSpec` (via
`ForwardIndexType.validate`):
+ /// well-formed specs on supported column shapes are accepted, and malformed
or type-incompatible
+ /// specs fail with precise errors.
+ @Test
+ public void testCodecSpecValidation() {
+ Schema schema = new Schema.SchemaBuilder().setSchemaName(TABLE_NAME)
+ .addSingleValueDimension("intCol", DataType.INT)
+ .addSingleValueDimension("longCol", DataType.LONG)
+ .addSingleValueDimension("stringCol", DataType.STRING)
+ .addMultiValueDimension("mvIntCol", DataType.INT)
+ .build();
+
+ // An unknown codec name inside indexes.forward.codecSpec fails with a
precise error.
+ TableConfig tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+
tableConfig.setFieldConfigList(List.of(rawFieldConfigWithCodecSpec("intCol",
"LZ4,UNKNOWN")));
+ TableConfig unknownCodecTableConfig = tableConfig;
+ Exception exception = expectThrows(Exception.class,
+ () -> TableConfigUtils.validate(unknownCodecTableConfig, schema));
+ assertTrue(exception.getMessage().contains("Unknown codec"), "Unexpected
error: " + exception.getMessage());
+
+ // A multi-stage codecSpec requires the V7 codec-pipeline writer, which
only supports
+ // single-value columns, so it is rejected on a multi-value column.
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+
tableConfig.setFieldConfigList(List.of(rawFieldConfigWithCodecSpec("mvIntCol",
"LZ4,SNAPPY")));
+ TableConfig mvChainTableConfig = tableConfig;
+ exception = expectThrows(Exception.class, () ->
TableConfigUtils.validate(mvChainTableConfig, schema));
+ assertTrue(exception.getMessage().contains("only supports single-value
columns")
+ && exception.getMessage().contains("mvIntCol"), "Unexpected error: " +
exception.getMessage());
+
+ // A transform codecSpec likewise requires the V7 writer, so DELTA on a
multi-value column is
+ // rejected even though the column's stored type (INT) is supported.
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+
tableConfig.setFieldConfigList(List.of(rawFieldConfigWithCodecSpec("mvIntCol",
"DELTA,LZ4")));
+ TableConfig mvTransformTableConfig = tableConfig;
+ exception = expectThrows(Exception.class, () ->
TableConfigUtils.validate(mvTransformTableConfig, schema));
+ assertTrue(exception.getMessage().contains("only supports single-value
columns")
+ && exception.getMessage().contains("mvIntCol"), "Unexpected error: " +
exception.getMessage());
+
+ // A V7-requiring spec on a single-value column of an unsupported stored
type (STRING) is
+ // rejected with the INT/LONG-only error. ZSTD(5) is compression-only but
its non-default level
+ // cannot be represented by a legacy ChunkCompressionType, so it needs the
V7 writer.
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+
tableConfig.setFieldConfigList(List.of(rawFieldConfigWithCodecSpec("stringCol",
"ZSTD(5)")));
+ TableConfig stringV7TableConfig = tableConfig;
+ exception = expectThrows(Exception.class, () ->
TableConfigUtils.validate(stringV7TableConfig, schema));
+ assertTrue(exception.getMessage().contains("only supports INT and LONG
columns")
+ && exception.getMessage().contains("stringCol"), "Unexpected error: "
+ exception.getMessage());
+
+ // A transform after a packing transform must be rejected (T64 output is
not a typed value
+ // array, so DELTA cannot consume it).
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+
tableConfig.setFieldConfigList(List.of(rawFieldConfigWithCodecSpec("intCol",
"T64,DELTA,LZ4")));
+ TableConfig misorderedTableConfig = tableConfig;
+ exception = expectThrows(Exception.class, () ->
TableConfigUtils.validate(misorderedTableConfig, schema));
+ assertTrue(exception.getMessage().contains("must operate on column
values"),
+ "Unexpected error: " + exception.getMessage());
+
+ // A disabled modern forward-index config cannot retain an ignored
codecSpec. This must be rejected even
+ // without the legacy FieldConfig.forwardIndexDisabled property.
+ ObjectNode disabledForward = JsonUtils.newObjectNode();
+ disabledForward.put("disabled", true);
+ disabledForward.put("codecSpec", "DELTA,LZ4");
+ ObjectNode disabledIndexes = JsonUtils.newObjectNode();
+ disabledIndexes.set("forward", disabledForward);
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+ tableConfig.setFieldConfigList(List.of(new FieldConfig.Builder("intCol")
+ .withEncodingType(FieldConfig.EncodingType.RAW)
+ .withIndexes(disabledIndexes)
+ .build()));
+ TableConfig disabledCodecSpecTableConfig = tableConfig;
+ exception = expectThrows(Exception.class,
+ () -> TableConfigUtils.validate(disabledCodecSpecTableConfig, schema));
+ assertTrue(exception.getMessage().contains("codecSpec cannot be configured
when the forward index is disabled")
+ && exception.getMessage().contains("intCol"), "Unexpected error: " +
exception.getMessage());
+
+ // A well-formed compression-only RAW codecSpec passes table-config
validation.
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+
tableConfig.setFieldConfigList(List.of(rawFieldConfigWithCodecSpec("intCol",
"ZSTD(3)")));
+ TableConfigUtils.validate(tableConfig, schema);
+
+ // A transform + compression chain on a RAW single-value INT column passes
validation.
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+
tableConfig.setFieldConfigList(List.of(rawFieldConfigWithCodecSpec("intCol",
"DELTA,ZSTD(3)")));
+ TableConfigUtils.validate(tableConfig, schema);
+
+ // A transform pipeline on a LONG column passes validation.
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+
tableConfig.setFieldConfigList(List.of(rawFieldConfigWithCodecSpec("longCol",
"DELTADELTA,LZ4")));
+ TableConfigUtils.validate(tableConfig, schema);
+
+ // Every non-null codecSpec routes through the V7 writer, including a
single compression stage. Reject
+ // unsupported stored types and multi-value columns consistently with
transform pipelines.
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+
tableConfig.setFieldConfigList(List.of(rawFieldConfigWithCodecSpec("stringCol",
"SNAPPY")));
+ TableConfig stringCompressionTableConfig = tableConfig;
+ exception = expectThrows(Exception.class, () ->
TableConfigUtils.validate(stringCompressionTableConfig, schema));
+ assertTrue(exception.getMessage().contains("only supports INT and LONG
columns")
+ && exception.getMessage().contains("stringCol"), "Unexpected error: "
+ exception.getMessage());
+
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+
tableConfig.setFieldConfigList(List.of(rawFieldConfigWithCodecSpec("mvIntCol",
"LZ4")));
+ TableConfig mvCompressionTableConfig = tableConfig;
+ exception = expectThrows(Exception.class, () ->
TableConfigUtils.validate(mvCompressionTableConfig, schema));
+ assertTrue(exception.getMessage().contains("only supports single-value
columns")
+ && exception.getMessage().contains("mvIntCol"), "Unexpected error: " +
exception.getMessage());
+
+ // Regression: codecSpec validation runs via `IndexType.validate(...)`
after
+ // `FieldIndexConfigsUtil` resolves overrides — not by an early
raw-FieldConfig pre-pass.
+ // A column whose RAW encoding is resolved from `noDictionaryColumns`,
with codecSpec set under
+ // `indexes.forward`, must pass validation.
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+ tableConfig.getIndexingConfig().setNoDictionaryColumns(List.of("intCol"));
+
tableConfig.setFieldConfigList(List.of(rawFieldConfigWithCodecSpec("intCol",
"DELTA,LZ4")));
+ TableConfigUtils.validate(tableConfig, schema);
Review Comment:
This case doesn't exercise what the comment describes.
`rawFieldConfigWithCodecSpec` sets `encodingType=RAW` on the `FieldConfig`, so
nothing is resolved from `noDictionaryColumns`; it is equivalent to the
positive `intCol` `DELTA,LZ4` case in `testCodecSpecTableConfigValidation` plus
a redundant `noDictionaryColumns` entry. The scenario described (default
DICTIONARY `FieldConfig` + `noDictionaryColumns` + `indexes.forward.codecSpec`)
is rejected earlier by `validateIndexingConfigAndFieldConfigListCompatibility`
("FieldConfig encoding type is different from indexingConfig for column"), so
it can't be a regression target. I'd drop the case, or assert that rejection
instead.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructIndexType.java:
##########
@@ -137,6 +137,11 @@ private void validatePerKeyIndexes(OpenStructIndexConfig
config) {
if (indexes == null) {
continue;
}
+ JsonNode forwardIndex =
indexes.get(StandardIndexes.forward().getPrettyName());
+ // The OPEN_STRUCT splitter builds its own per-key forward-index configs
(dict-vs-raw decision plus a
+ // fixed LZ4 raw compression), so a per-key codecSpec would be silently
discarded. Reject it explicitly.
+ Preconditions.checkState(forwardIndex == null ||
!forwardIndex.hasNonNull("codecSpec"),
+ "codecSpec is not supported for OPEN_STRUCT key: %s",
fieldConfig.getName());
Review Comment:
Nit: for `defaultValueFieldConfig` this reports `OPEN_STRUCT key: default`,
i.e. the config's name, which isn't a key, and the parent column isn't named.
Other messages in this validator use `OPEN_STRUCT column '%s': key '%s' ...`;
consider passing the parent column into `validatePerKeyIndexes` and naming
`defaultValueFieldConfig` for the default case.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1798,6 +1798,145 @@ public void testValidateFieldConfig() {
}
}
+ /// Semantic table-config-time validation of `codecSpec` (via
`ForwardIndexType.validate`):
+ /// well-formed specs on supported column shapes are accepted, and malformed
or type-incompatible
+ /// specs fail with precise errors.
+ @Test
+ public void testCodecSpecValidation() {
+ Schema schema = new Schema.SchemaBuilder().setSchemaName(TABLE_NAME)
+ .addSingleValueDimension("intCol", DataType.INT)
+ .addSingleValueDimension("longCol", DataType.LONG)
+ .addSingleValueDimension("stringCol", DataType.STRING)
+ .addMultiValueDimension("mvIntCol", DataType.INT)
+ .build();
+
+ // An unknown codec name inside indexes.forward.codecSpec fails with a
precise error.
+ TableConfig tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+
tableConfig.setFieldConfigList(List.of(rawFieldConfigWithCodecSpec("intCol",
"LZ4,UNKNOWN")));
+ TableConfig unknownCodecTableConfig = tableConfig;
+ Exception exception = expectThrows(Exception.class,
+ () -> TableConfigUtils.validate(unknownCodecTableConfig, schema));
+ assertTrue(exception.getMessage().contains("Unknown codec"), "Unexpected
error: " + exception.getMessage());
+
+ // A multi-stage codecSpec requires the V7 codec-pipeline writer, which
only supports
+ // single-value columns, so it is rejected on a multi-value column.
Review Comment:
Stale comment: every non-null `codecSpec` requires the V7 writer, not only
multi-stage specs (line 1891 of this test says so, and MV `LZ4` is rejected the
same way at line 1901). Something like "Every codecSpec uses the V7 writer,
which only supports single-value columns" would match the code.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -1798,6 +1798,145 @@ public void testValidateFieldConfig() {
}
}
+ /// Semantic table-config-time validation of `codecSpec` (via
`ForwardIndexType.validate`):
+ /// well-formed specs on supported column shapes are accepted, and malformed
or type-incompatible
+ /// specs fail with precise errors.
+ @Test
+ public void testCodecSpecValidation() {
+ Schema schema = new Schema.SchemaBuilder().setSchemaName(TABLE_NAME)
+ .addSingleValueDimension("intCol", DataType.INT)
+ .addSingleValueDimension("longCol", DataType.LONG)
+ .addSingleValueDimension("stringCol", DataType.STRING)
+ .addMultiValueDimension("mvIntCol", DataType.INT)
+ .build();
+
+ // An unknown codec name inside indexes.forward.codecSpec fails with a
precise error.
+ TableConfig tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+
tableConfig.setFieldConfigList(List.of(rawFieldConfigWithCodecSpec("intCol",
"LZ4,UNKNOWN")));
+ TableConfig unknownCodecTableConfig = tableConfig;
+ Exception exception = expectThrows(Exception.class,
+ () -> TableConfigUtils.validate(unknownCodecTableConfig, schema));
+ assertTrue(exception.getMessage().contains("Unknown codec"), "Unexpected
error: " + exception.getMessage());
+
+ // A multi-stage codecSpec requires the V7 codec-pipeline writer, which
only supports
+ // single-value columns, so it is rejected on a multi-value column.
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+
tableConfig.setFieldConfigList(List.of(rawFieldConfigWithCodecSpec("mvIntCol",
"LZ4,SNAPPY")));
+ TableConfig mvChainTableConfig = tableConfig;
+ exception = expectThrows(Exception.class, () ->
TableConfigUtils.validate(mvChainTableConfig, schema));
+ assertTrue(exception.getMessage().contains("only supports single-value
columns")
+ && exception.getMessage().contains("mvIntCol"), "Unexpected error: " +
exception.getMessage());
+
+ // A transform codecSpec likewise requires the V7 writer, so DELTA on a
multi-value column is
+ // rejected even though the column's stored type (INT) is supported.
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+
tableConfig.setFieldConfigList(List.of(rawFieldConfigWithCodecSpec("mvIntCol",
"DELTA,LZ4")));
+ TableConfig mvTransformTableConfig = tableConfig;
+ exception = expectThrows(Exception.class, () ->
TableConfigUtils.validate(mvTransformTableConfig, schema));
+ assertTrue(exception.getMessage().contains("only supports single-value
columns")
+ && exception.getMessage().contains("mvIntCol"), "Unexpected error: " +
exception.getMessage());
+
+ // A V7-requiring spec on a single-value column of an unsupported stored
type (STRING) is
+ // rejected with the INT/LONG-only error. ZSTD(5) is compression-only but
its non-default level
+ // cannot be represented by a legacy ChunkCompressionType, so it needs the
V7 writer.
Review Comment:
Stale comment: `ZSTD(5)` needs V7 because every `codecSpec` does, not
because of its level; `ZSTD(3)` or `SNAPPY` on this STRING column fail with the
same error (see the `SNAPPY` case below). This reads like the earlier design
where legacy-representable specs stayed on the legacy format.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1781,6 +1781,12 @@ private static void
validateIndexingConfigAndFieldConfigList(TableConfig tableCo
// Validate DELTA / DELTADELTA compression codecs compatibility
validateGorillaCompressionCodecIfPresent(fieldConfig,
schema.getFieldSpecFor(column));
+
+ // Note: codecSpec is validated below via `indexType.validate(...)`
once FieldIndexConfigsUtil
+ // has resolved the effective ForwardIndexConfig (merging the legacy
`noDictionaryColumns` /
+ // `noDictionaryConfig` signals into the resolved encoding type).
Validating the raw FieldConfig
+ // here would incorrectly reject legacy tables that express RAW via
`noDictionaryColumns` while
+ // configuring codecSpec under `indexes.forward`.
Review Comment:
The premise here doesn't hold: a table that expresses RAW via
`noDictionaryColumns` must also set `FieldConfig.encodingType=RAW`, otherwise
`validateIndexingConfigAndFieldConfigListCompatibility` (just below) rejects
it. So checking `FieldConfig.encodingType` up front would not wrongly reject
such a table. Since this block adds no code, I'd drop the note, or reduce it to
"codecSpec is validated by `ForwardIndexType.validate` on the resolved
`ForwardIndexConfig`".
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexTypeTest.java:
##########
@@ -495,4 +502,32 @@ public void testStandardIndex() {
assertSame(StandardIndexes.forward(), StandardIndexes.forward(), "Standard
index should use the same as "
+ "the ForwardIndexType static instance");
}
+
+ /// codecSpec applies only at immutable segment creation/conversion time.
The mutable (consuming)
+ /// forward index must build the standard in-memory format, ignoring the
configured codecSpec, so
+ /// realtime tables with a codecSpec keep consuming normally.
+ @Test
+ public void testCodecSpecBuildsStandardMutableIndexForRealtime()
+ throws Exception {
+ MutableIndexContext context = Mockito.mock(MutableIndexContext.class);
+ Mockito.when(context.getFieldSpec()).thenReturn(
+ new DimensionFieldSpec("dimInt", FieldSpec.DataType.INT, true));
+ Mockito.when(context.getSegmentName()).thenReturn("testSegment");
+ Mockito.when(context.getCapacity()).thenReturn(16);
Review Comment:
Nit (AGENTS.md test conventions): statically import `mock` / `when` instead
of qualifying with `Mockito.`, and import `FieldSpec.DataType` directly
(`DataType.INT`).
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/openstruct/OpenStructColumnSplitter.java:
##########
@@ -408,6 +409,12 @@ private void writeDenseKeyColumn(String key)
boolean useDictionary = resolveUseDictionary(childFieldSpec,
configsForDecision, statsCollector);
+ // Defense-in-depth mirror of OpenStructIndexType.validatePerKeyIndexes:
the child forward config built
+ // below discards any per-key codecSpec, so refuse to silently drop one
that slipped past validation.
+ ForwardIndexConfig configuredForwardIndex =
configsForDecision.getConfig(StandardIndexes.forward());
+ Preconditions.checkState(configuredForwardIndex.getCodecSpec() == null,
+ "codecSpec is not supported for OPEN_STRUCT key: %s", key);
Review Comment:
Follow-up, not introduced here: `codecSpec` is now the only per-key forward
setting that is rejected. The child forward config built below still silently
drops per-key `compressionCodec` / `chunkCompressionType`,
`targetDocsPerChunk`, `rawIndexWriterVersion`, etc. (raw children are always
LZ4). Consider rejecting those in `validatePerKeyIndexes` as well, or
documenting that OPEN_STRUCT children ignore forward-index tuning.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/fwd/SingleValueFixedByteRawIndexCreator.java:
##########
@@ -138,20 +148,27 @@ public void close()
@Override
public long getRawForwardIndexUncompressedValueSizeInBytes() {
- // Compression-statistics metadata supports only the legacy
single-compressor format.
if (_indexWriter instanceof FixedByteChunkForwardIndexWriter legacyWriter)
{
return legacyWriter.getRawForwardIndexUncompressedValueSizeInBytes();
}
+ // V7 codec-pipeline writer: the fixed-byte layout stores exactly one
value per doc, so the
+ // uncompressed size is totalDocs * valueType.size(). Report it only when
tracking was requested,
+ // matching the legacy writer's contract of returning -1 otherwise.
+ if (_trackUncompressedValueSize) {
+ return (long) _totalDocs * _valueType.size();
+ }
Review Comment:
Optional: this uses the declared `totalDocs` rather than the values actually
written. It's exact for every loadable file, since
`FixedByteChunkForwardIndexWriterV7.close()` only writes the header when
`docsWritten == totalDocs`, whereas the legacy writer reports bytes actually
written (including the in-flight chunk). Exposing the V7 writer's written-doc
count and using that would keep the two contracts identical if this is ever
read mid-write.
--
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]
