xiangfu0 commented on code in PR #19307:
URL: https://github.com/apache/pinot/pull/19307#discussion_r4109727304
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/TextIndexHandler.java:
##########
@@ -373,7 +373,7 @@ private void
convertTextIndexToV3Format(SegmentDirectory.Writer segmentWriter, S
return;
}
- // Write the combined file to V3 format (similar to
rewriteForwardIndexForCompressionChange)
+ // Write the combined file to V3 format (similar to
ForwardIndexHandler.rewriteForwardIndex)
Review Comment:
Dropped — `TextIndexHandler.java` is now byte-identical to master, so it is
out of this PR entirely (the file no longer appears in the diff for #19307,
#19308 or #19309).
One consequence worth naming: that comment referenced
`rewriteForwardIndexForCompressionChange`, which this PR renames to
`ForwardIndexHandler.rewriteForwardIndex`, so with the edit dropped the comment
now names a method that no longer exists on the stack. It is a dangling doc
reference only — no code reference and no compile impact (a grep for the old
name across the stack returns just that one comment). I did not rename the
method back. Happy to fix the wording in a one-line PR straight onto master,
which keeps it out of this one.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/RawForwardIndexWithDictionaryTest.java:
##########
@@ -178,7 +188,34 @@ protected List<FieldConfig> getFieldConfigs() {
FieldConfig rawDictRangeInt =
new FieldConfig(RAW_DICT_RANGE_INT_DIMENSION,
FieldConfig.EncodingType.RAW, null, null, null, null,
dictionaryIndex.deepCopy(), null, null);
- return List.of(rawDictString, rawDictInvString, rawDictInt, rawDictInvInt,
rawDictRangeInt);
+ return List.of(rawDictString, rawDictInvString, rawDictInt, rawDictInvInt,
rawDictRangeInt,
+ codecFieldConfig(CODEC_INT_DIMENSION, "DELTA,T64,LZ4"),
+ codecFieldConfig(CODEC_LONG_DIMENSION, "ZSTD(3)"));
+ }
+
+ private static FieldConfig codecFieldConfig(String column, String codecSpec)
{
+ ObjectNode forward = JsonUtils.newObjectNode().put("codecSpec",
codecSpec).put("targetDocsPerChunk", 128);
+ ObjectNode indexes = JsonUtils.newObjectNode();
+ indexes.set("forward", forward);
+ return new
FieldConfig.Builder(column).withEncodingType(FieldConfig.EncodingType.RAW).withIndexes(indexes).build();
+ }
+
+ /// Exercises persisted INT/LONG pipelines through ingestion and both query
engines, across chunk boundaries.
Review Comment:
Done — `RawForwardIndexWithDictionaryTest` is restored to master
byte-for-byte, so the dictionary-focused fixture is untouched. That removes the
two codec columns, their schema/Avro/record wiring, the `codecFieldConfig`
helper and `testCodecPipelineQueries`.
I checked #19309 covers what was here rather than assuming it: its
`CODEC_SPECS` map drives a dedicated INT and LONG column per spec through both
engines, with `NUM_DOCS=1000` against `targetDocsPerChunk=256` and point-lookup
ids straddling the 512 boundary, a dict-encoded STRING column asserted to stay
DICTIONARY, and `testGeneratedSegmentsUseConfiguredForwardIndexFormats`
asserting the persisted spec round-trips per column. The one spec exercised
here that its map lacked was the three-stage `DELTA,T64,LZ4` (it had
`DELTA,LZ4`, `T64` and `T64,LZ4`), so I added that single entry there — it
flows through the schema/Avro/field-config generation and the per-codec
SUM/filter/point-lookup providers automatically, so the coverage genuinely
moves rather than disappearing. The V7 format itself stays covered by the
focused round-trip tests in this PR.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexCreatorFactoryTest.java:
##########
@@ -105,21 +121,448 @@ public void
testTableConfigConstructorPropagatesCompressionStatsFlag()
}
}
+ @DataProvider(name = "v7CodecSpecs")
+ public Object[][] v7CodecSpecs() {
+ return new Object[][]{
+ {"LZ4", DataType.INT}, {"DELTA,LZ4", DataType.INT},
+ {"ZSTD(3)", DataType.LONG}, {"DELTADELTA,GORILLA,ZSTD(3)",
DataType.LONG}
+ };
+ }
+
+ /// Both compression-only and transform pipelines use V7, including a
partial final chunk.
+ @Test(dataProvider = "v7CodecSpecs")
+ public void testCodecSpecRoundTripUsesV7Format(String codecSpec, DataType
storedType)
+ throws Exception {
+ File indexDir =
Files.createTempDirectory("ForwardIndexCreatorFactoryTest").toFile();
+ try {
+ ForwardIndexConfig config = new
ForwardIndexConfig.Builder(FieldConfig.EncodingType.RAW)
+ .withCodecSpec(codecSpec)
+ .withTargetDocsPerChunk(2)
+ .build();
+ TableConfig tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build();
+ // Legacy compression stats are opted in here on purpose: a codecSpec
column never reports them.
+ tableConfig.getIndexingConfig().setCompressionStatsEnabled(true);
+ long[] values = storedType == DataType.INT
+ ? new long[]{11, 13, 21}
+ : new long[]{Long.MIN_VALUE, (long) Integer.MAX_VALUE + 1,
Long.MAX_VALUE};
+ try (ForwardIndexCreator creator =
ForwardIndexCreatorFactory.createIndexCreator(
+ newContext(indexDir, false, tableConfig, values.length, storedType),
config)) {
+ assertFalse(creator.isDictionaryEncoded());
+ assertNull(creator.getRawForwardIndexChunkCompressionType());
+ for (long value : values) {
+ if (storedType == DataType.INT) {
+ creator.putInt((int) value);
+ } else {
+ creator.putLong(value);
+ }
+ }
+ creator.seal();
+ assertEquals(creator.getRawForwardIndexUncompressedValueSizeInBytes(),
-1L);
+ }
+ File indexFile = new File(indexDir, COLUMN_NAME +
V1Constants.Indexes.RAW_SV_FORWARD_INDEX_FILE_EXTENSION);
+ assertTrue(indexFile.exists());
+ try (PinotDataBuffer buffer =
PinotDataBuffer.mapReadOnlyBigEndianFile(indexFile);
+ ForwardIndexReader<?> reader =
+
ForwardIndexReaderFactory.getInstance().createRawIndexReader(buffer,
storedType, true)) {
+ assertTrue(reader instanceof FixedByteChunkSVForwardIndexReaderV7,
+ "codecSpec was routed to " + reader.getClass().getSimpleName());
+ assertEquals(buffer.getInt(0),
FixedByteChunkSVForwardIndexReaderV7.VERSION);
+
assertEquals(FixedByteChunkSVForwardIndexReaderV7.readCodecSpec(buffer),
codecSpec);
+ FixedByteChunkSVForwardIndexReaderV7 v7Reader =
(FixedByteChunkSVForwardIndexReaderV7) reader;
+ try (FixedByteChunkSVForwardIndexReaderV7.Context context =
v7Reader.createContext()) {
+ for (int i = 0; i < values.length; i++) {
+ assertEquals(storedType == DataType.INT ? v7Reader.getInt(i,
context) : v7Reader.getLong(i, context),
+ values[i]);
+ }
+ }
+ }
+ } finally {
+ FileUtils.deleteQuietly(indexDir);
+ }
+ }
+
+ /// One multi-stage pipeline per stored type, plus a single-stage spec so
the executor's
+ /// multi-stage-only scratch slots and the single-stage path both run at
chunk-boundary scale.
+ /// Every spec in `v7CodecSpecs` is already round-tripped, including a
partial final chunk, by
+ /// [#testCodecSpecRoundTripUsesV7Format] above.
+ @DataProvider(name = "v7MultiChunkCodecSpecs")
+ public Object[][] v7MultiChunkCodecSpecs() {
+ return new Object[][]{
+ {"LZ4", DataType.INT}, {"DELTA,LZ4", DataType.INT},
{"DELTADELTA,GORILLA,ZSTD(3)", DataType.LONG}
+ };
+ }
+
+ /// Realistic-volume round trip: many chunks, a non-power-of-two target
normalized to 1024 docs per
Review Comment:
Consolidated. The shared setup/write/read now lives in one private helper —
config construction, the table config, the creator write loop with its INT/LONG
branch, seal, the index-file mapping, `createRawIndexReader`, the V7 routing
assertion and the sequential read-back. Both tests call it, and the file is 11
lines shorter (568 to 557).
Both distinct behaviours you named are kept: the power-of-two normalization
assertions stay in the multi-chunk test verbatim (`targetDocsPerChunk=1000`
normalizing to 1024, plus numChunks and totalDocs from the header), and the
random-access path keeps its two separate Contexts with the 2000 random reads.
The round-trip test keeps its own assertions too (version int, `readCodecSpec`
round-trip, null chunk-compression type, the uncompressed-size assertion under
`compressionStatsEnabled`, and the extreme LONG values).
Two side effects worth knowing. Routing both tests through the helper means
the creator-contract assertions now also apply to the 10009-doc multi-chunk
case, which previously asserted none of them and hard-cast the reader. And
because #19308 changes the uncompressed-size expectation for V7, the helper
asserts both branches (enabled gives `totalDocs * size()`, disabled gives -1),
so that PR gains the disabled-branch assertion it did not have. Test-method and
invocation counts are unchanged at 11/26, and the other tests in the file are
untouched.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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]
