Jackie-Jiang commented on code in PR #19307:
URL: https://github.com/apache/pinot/pull/19307#discussion_r4109600055
##########
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:
This is a comment-only edit in the unrelated text-index handler. Please drop
it to keep this PR focused.
##########
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:
Could we move this codec-specific integration coverage to #19309 and leave
the dictionary-focused fixture unchanged? That PR already tests codec ingestion
and queries through both query engines, including chunk boundaries and
dictionary coexistence. The focused V7 round-trip tests in this PR still cover
the format itself.
##########
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:
The round-trip test above already crosses a chunk boundary and reads a
partial final chunk. Could we consolidate the shared setup/write/read here,
retaining the distinct power-of-two normalization and random-access assertions?
That would keep the coverage while reducing this large test addition.
--
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]
