You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "JingsongLi (via GitHub)" <gi...@apache.org> on 2026/08/28 13:00:38 UTC
Re: [PR] [core] Harden and slim FM index storage [paimon]
JingsongLi commented on code in PR #9450:
URL: https://github.com/apache/paimon/pull/9450#discussion_r3880760065
##########
paimon-common/src/test/java/org/apache/paimon/globalindex/fmindex/FMGlobalIndexTest.java:
##########
@@ -469,6 +476,102 @@ public void testReadCostOptionsAreValidated() {
.hasMessageContaining("demand page size");
}
+ @Test
+ public void testZeroReadCacheDeclinesLocateInsteadOfRereadingBlocks() throws Exception {
+ Options options = new Options();
+ options.set(FMGlobalIndexOptions.PARTITION_ROW_COUNT, 10);
+ options.set(FMGlobalIndexOptions.SA_SAMPLE_RATE, 1024);
+ options.set(FMGlobalIndexOptions.COMPRESSION, "none");
+ options.set(FMGlobalIndexOptions.READ_CACHE_SIZE, MemorySize.ofBytes(0));
+ options.set(FMGlobalIndexOptions.LOCATE_COST_RATIO, 1d);
+ indexer = new FMGlobalIndexer(dataField, options);
+ String value = String.join("", Collections.nCopies(3_000, "x")) + "unique-needle";
+ List<GlobalIndexIOMeta> files = writeData(Collections.singletonList(str(value)), 0);
+ GlobalIndexIOMeta file = files.get(0);
+ long sampleBlockOffset;
+ try (SeekableInputStream input = fileIO.newInputStream(file.filePath())) {
+ FMIndexFile.Footer footer = FMIndexFile.readFooter(input, file.fileSize());
+ sampleBlockOffset =
+ FMIndexFile.readDirectory(input, footer, file.fileSize())
+ .sampleValues
+ .blocks
+ .get(0)
+ .block
+ .offset;
+ }
+ corruptByte(file, sampleBlockOffset);
+
+ try (GlobalIndexReader reader = createReader(files, 1)) {
+ // With sufficient cache this selective interval uses SA locate and observes the
+ // corrupted sample. A zero cache must take the exact-value path without LF rereads.
+ assertRows(reader.visitContains(fieldRef, str("unique-needle")).join(), 0L);
+ }
+ }
+
+ @Test
+ public void testZeroReadCacheScansNullBitmapOncePerBlock() throws Exception {
+ Options options = new Options();
+ options.set(FMGlobalIndexOptions.PARTITION_ROW_COUNT, 1_000);
+ options.set(FMGlobalIndexOptions.COMPRESSION, "none");
+ options.set(FMGlobalIndexOptions.READ_CACHE_SIZE, MemorySize.ofBytes(0));
+ indexer = new FMGlobalIndexer(dataField, options);
+ List<BinaryString> values = new ArrayList<>();
+ for (int row = 0; row < 1_000; row++) {
+ values.add((row & 1) == 0 ? null : str("value-" + row));
+ }
+ List<GlobalIndexIOMeta> files = writeData(values, 0);
+
+ AtomicInteger preadCalls = new AtomicInteger();
+ fileReader =
+ meta ->
+ new CountingVectoredInput(
+ fileIO.newInputStream(meta.filePath()), preadCalls);
+ try (GlobalIndexReader reader = createReader(files, values.size())) {
+ assertRows(
+ reader.visitIsNull(fieldRef).join(),
+ java.util.stream.LongStream.range(0, values.size())
+ .filter(row -> (row & 1) == 0)
+ .toArray());
+ }
+ assertThat(preadCalls.get()).isLessThan(20);
+ }
+
+ @Test
+ public void testVerificationValuesCanBeOmitted() throws Exception {
Review Comment:
Addressed in f1d9b5ca55. The assertion is now tightened to fewer than 10 physical reads, while still allowing footer and directory reads around the single null-bitmap block.
##########
paimon-common/src/main/java/org/apache/paimon/globalindex/fmindex/FMIndexReadContext.java:
##########
@@ -32,6 +32,11 @@
final class FMIndexReadContext {
private static final int DEFAULT_MAX_CONCURRENT_FILE_READS = 8;
+ // A locate must at least be able to retain one decoded rank/sample block. Otherwise every
Review Comment:
Addressed in f1d9b5ca55. I split the formula into named packed-word and rank-prefix components. One nuance: both terms belong to a single decoded quaternary rank block; this is the largest block touched by locate and is larger than the bit-rank and sampled-value blocks.
##########
paimon-common/src/main/java/org/apache/paimon/globalindex/fmindex/FMIndexFile.java:
##########
@@ -1209,20 +1218,28 @@ private static int[] waveletStarts(int[] cumulative, int levelCount) {
}
private static int footerChecksum(byte[] footer) {
- return crc32c(
- new MemorySlice(MemorySegment.wrap(footer), 0, FOOTER_CHECKSUM_OFFSET),
- BlockCompressionType.NONE);
+ return crc32(footer, 0, FOOTER_CHECKSUM_OFFSET, BlockCompressionType.NONE);
}
private static int indexMetaChecksum(byte[] metadata) {
- return crc32c(
- new MemorySlice(
- MemorySegment.wrap(metadata),
- 0,
- metadata.length - INDEX_META_CHECKSUM_LENGTH),
+ return crc32(
+ metadata,
Review Comment:
Thanks. Despite the helper name, the previous `SstFileUtils.crc32c` implementation uses `java.util.zip.CRC32`, so this refactor does not change persisted checksum values; the regression test pins the IEEE CRC32 vector. Also, the FM V1 format has not shipped in any release, so there is no released file compatibility boundary that requires a version bump. We are intentionally keeping `VERSION = 1` and refreshing the golden fixture before the first release.
##########
paimon-common/src/main/java/org/apache/paimon/globalindex/fmindex/FMGlobalIndexReader.java:
##########
@@ -273,9 +292,12 @@ private boolean shouldLocate(
Metadata metadata,
SearchInterval interval,
Review Comment:
This became stale after the follow-up alignment with Milvus. Current HEAD removed verification-value storage entirely; `shouldLocate` now only checks cache support and locate cost versus source text bytes. Expensive intervals return unsupported so the normal data path scans source values.
##########
paimon-common/src/main/java/org/apache/paimon/globalindex/fmindex/FMGlobalIndexWriter.java:
##########
@@ -93,12 +96,14 @@ public void write(@Nullable Object key, long relativeRowId) {
"FM index expects BinaryString values, but found %s.",
key.getClass().getName());
bytes = ((BinaryString) key).toBytes();
- Preconditions.checkArgument(
- bytes.length
- <= FMIndexFile.MAX_VERIFICATION_BLOCK_UNCOMPRESSED_LENGTH
- - Integer.BYTES,
- "A value exceeds the FM index exact-fallback block limit (%s bytes).",
- FMIndexFile.MAX_VERIFICATION_BLOCK_UNCOMPRESSED_LENGTH);
+ if (storeVerificationValues) {
Review Comment:
This became stale after the follow-up alignment with Milvus. Current HEAD removed `storeVerificationValues`, verification blocks, and the associated value-length guard entirely, so there is no optional branch left to document.
##########
paimon-common/src/main/java/org/apache/paimon/globalindex/fmindex/FMGlobalIndexReader.java:
##########
@@ -181,6 +181,22 @@ private Optional<GlobalIndexResult> queryUnchecked(
current.footer.firstRowId + current.footer.rowCount)) {
return exactEmptyResult();
}
+ boolean hasNonEmptyNeedle = false;
Review Comment:
Addressed in f1d9b5ca55 with the explicit `if` plus `break` short-circuit form.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org