J-HowHuang commented on code in PR #18998:
URL: https://github.com/apache/pinot/pull/18998#discussion_r3597708058
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandlerTest.java:
##########
@@ -1771,6 +1768,119 @@ public void testDisableDictionaryForSortedColumn()
metadata, false);
}
+ @DataProvider(name = "coincidentallySortedRawTypes")
+ public Object[][] coincidentallySortedRawTypes() {
+ // Cover a fixed-width type and both variable-length types (STRING/BYTES
take a different raw writer path).
+ return new Object[][]{
+ {DataType.LONG, 0L},
+ {DataType.STRING, "sameValue"},
+ {DataType.BYTES, new byte[]{0x1, 0x2, 0x3}},
+ };
+ }
+
+ /// Regression test for enabling a dictionary on a raw column whose
persisted metadata says {@code isSorted=false}
+ /// but whose values happen to be monotonic (here, all identical). This is
the shape produced by a realtime segment
+ /// committed while the column was raw: raw columns are persisted with
{@code isSorted=false} regardless of the data.
+ /// On reload the fresh stats collector reads the identical values and
reports the column as sorted, so before the
+ /// fix the forward-index creator emitted a `.sv.sorted.fwd` index while
{@link ForwardIndexHandler} derived the temp
+ /// file name from the metadata ({@code .sv.unsorted.fwd}) and never updated
{@code isSorted}, throwing
+ /// {@link java.io.FileNotFoundException} and leaving a format/metadata
mismatch for the next read. The fix forces
+ /// the creator to honor the persisted {@code isSorted} flag, keeping the
written format, the file name, and the
+ /// metadata consistent.
+ @Test(dataProvider = "coincidentallySortedRawTypes")
+ public void
testEnableDictionaryForRawColumnWithCoincidentallySortedValues(DataType
dataType, Object value)
+ throws Exception {
+ File tempDir = new File(FileUtils.getTempDirectory(),
"ForwardIndexHandlerCoincidentalSortTest");
+ FileUtils.deleteQuietly(tempDir);
+ try {
+ String tableName = "coincidentalSortTable";
+ String column = "coincidentallySortedColumn";
+ String segmentName = "coincidentalSortSegment";
+ int numDocs = 48;
+
+ Schema schema = new Schema.SchemaBuilder().setSchemaName(tableName)
+ .addSingleValueDimension(column, dataType)
+ .build();
+
+ // Build the segment with the column as raw (no dictionary).
+ TableConfig rawTableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(tableName)
+ .setNoDictionaryColumns(List.of(column))
+ .setFieldConfigList(
+ List.of(new FieldConfig(column, FieldConfig.EncodingType.RAW,
List.of(), CompressionCodec.PASS_THROUGH,
+ null)))
+ .build();
+
+ // All-identical values -> trivially monotonic -> the stats collector
treats the column as sorted.
+ List<GenericRow> rows = new ArrayList<>();
+ for (int i = 0; i < numDocs; i++) {
+ GenericRow row = new GenericRow();
+ row.putValue(column, value);
+ rows.add(row);
+ }
+ SegmentGeneratorConfig config = new
SegmentGeneratorConfig(rawTableConfig, schema);
+ config.setOutDir(tempDir.getPath());
+ config.setSegmentName(segmentName);
+ SegmentIndexCreationDriverImpl driver = new
SegmentIndexCreationDriverImpl();
+ driver.init(config, new GenericRowRecordReader(rows));
+ driver.build();
+ File segmentDir = new File(tempDir, segmentName);
+
+ // Offline generation records isSorted from the data, so it would set
isSorted=true for these all-equal values.
+ // Overwrite it to false to reproduce the realtime raw-segment condition.
+ PropertiesConfiguration metadataConfig =
SegmentMetadataUtils.getPropertiesConfiguration(segmentDir);
+ metadataConfig.setProperty(getKeyFor(column, IS_SORTED),
String.valueOf(false));
+ SegmentMetadataUtils.savePropertiesConfiguration(metadataConfig,
segmentDir);
+
+ ColumnMetadata beforeMetadata = new
SegmentMetadataImpl(segmentDir).getColumnMetadataFor(column);
+ assertFalse(beforeMetadata.isSorted());
+ assertFalse(beforeMetadata.hasDictionary());
+
+ // Enable a dictionary on the column and run the forward-index handler
(the reload path).
+ TableConfig dictTableConfig =
+ new
TableConfigBuilder(TableType.OFFLINE).setTableName(tableName).setNoDictionaryColumns(List.of()).build();
+ IndexLoadingConfig indexLoadingConfig = new
IndexLoadingConfig(dictTableConfig, schema);
+ try (SegmentDirectory segmentDirectory = new
SegmentLocalFSDirectory(segmentDir, ReadMode.mmap);
+ SegmentDirectory.Writer writer = segmentDirectory.createWriter()) {
+ ForwardIndexHandler handler = new
ForwardIndexHandler(segmentDirectory, indexLoadingConfig);
+ assertEquals(handler.computeOperations(writer),
+ Map.of(column,
List.of(ForwardIndexHandler.Operation.ENABLE_DICTIONARY)));
+ // Before the fix this threw FileNotFoundException for the missing
.sv.unsorted.fwd file.
+ handler.updateIndices(writer);
+ handler.postUpdateIndicesCleanup(writer);
+ }
+
+ // The column now has a dictionary; isSorted must stay false and the
unsorted dict forward index must read back
Review Comment:
Got it. So now
1. the file extension is determined based on what
`statsCollector.isSorted()` report, instead of from metadata
2. update the `isSorted` in metadata to `statsCollector.isSorted()`
--
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]
