utafrali commented on code in PR #9450:
URL: https://github.com/apache/paimon/pull/9450#discussion_r3880631446
##########
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:
The switch from `crc32c` (Castagnoli) to standard IEEE CRC32 produces
completely different values for the same input — any file written by the old
code will fail checksum verification under the new code, and vice versa. The
`VERSION` constant is written to the footer but does not appear to have been
incremented here.
If in-flight or already-written files exist (even in CI/CD artifact caches),
readers will reject them with a spurious checksum mismatch that is nearly
impossible to diagnose. Either bump `VERSION` so the reader can detect and
reject old-format files with a clear error message, or add an explicit note in
the class Javadoc that this change is only safe because no stable release
shipped the CRC32C format.
Suggested minimum: `private static final int VERSION = 2;` paired with a
guard in `readFooter` and `readContainerFooter` that rejects old version
numbers with a migration hint.
##########
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:
The `MIN_LOCATE_CACHE_BYTES` formula is correct but opaque. The two additive
terms correspond to different block types and it is not clear which is which.
Consider naming the components:
```java
// One decoded rank block: BLOCK_WORDS longs.
long rankBlockBytes = FMIndexFile.BLOCK_WORDS * Long.BYTES;
// One decoded sample block: one int per 64-bit word of the sampled-SA
bitmask, plus one overflow slot.
long sampleBlockBytes = ((FMIndexFile.BLOCK_WORDS + 63L) / 64L + 1L) * 4L *
Integer.BYTES;
private static final long MIN_LOCATE_CACHE_BYTES = rankBlockBytes +
sampleBlockBytes;
```
This makes the intent clear without changing behaviour.
##########
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:
The value-length guard (`bytes.length <=
MAX_VERIFICATION_BLOCK_UNCOMPRESSED_LENGTH - Integer.BYTES`) is now skipped
when `storeVerificationValues` is false. This is intentional — values that will
never be packed into verification blocks have no length limit — but a follow-up
`checkArgument` or comment on the outer block would make the intent clear to
the next reader:
```java
if (storeVerificationValues) {
// Values must fit in a single verification block together with their
4-byte length prefix.
Preconditions.checkArgument(
bytes.length <= ...,
"A value exceeds the FM index exact-fallback block limit (%s
bytes).",
...);
}
```
Without a comment someone may re-add the check unconditionally thinking it
was accidentally omitted.
##########
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:
`hasNonEmptyNeedle |= needle.length > 0;` uses bitwise-OR assignment on a
`boolean`. This works in Java but does not short-circuit: once
`hasNonEmptyNeedle` is `true` the remaining iterations still execute. For
typical needle counts the cost is negligible, but the idiom is non-standard and
surprises readers.
Prefer the explicit short-circuit form:
```java
if (needle.length > 0) {
hasNonEmptyNeedle = true;
break;
}
```
##########
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:
`assertThat(preadCalls.get()).isLessThan(20)` is a very loose bound. With 1
000 rows and `BLOCK_BITS = 4096 * 64 = 262 144`, all 1 000 null-bitmap bits fit
in a single bit block, so the block-wise scan should produce a handful of
physical reads (one seek + read per block, plus directory/footer reads). A
bound of, say, 10 would still give the test room to breathe on all compression
configurations while actually catching a regression to the old row-by-row path
(which would produce ~1 000 reads). Consider tightening the assertion.
##########
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:
The new early-return path when `!readContext.supportsLocate()` in
`shouldLocate()` is correct, but the surrounding cost calculation (`locateCost`
vs `verificationBytes` comparison) is now only reached when locate IS supported
AND verification pages are non-empty. A brief comment would clarify why the two
conditions were added and how they interact:
```java
// locate requires sufficient cache; skip cost comparison if we cannot
locate at all.
if (!readContext.supportsLocate()) {
return false;
}
// Cost comparison only applies when stored values are available for
fallback.
if (candidates != null && !directory.verificationPages.isEmpty()) {
```
--
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]
