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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
