leaves12138 commented on code in PR #8857:
URL: https://github.com/apache/paimon/pull/8857#discussion_r3655153671
##########
paimon-common/src/main/java/org/apache/paimon/lookup/sort/db/LocalKvDb.java:
##########
@@ -549,7 +607,8 @@ private SstFileMetadata
findFileForKey(List<SstFileMetadata> sortedFiles, Memory
private SstFileMetadata writeMemTableToSst(TreeMap<MemorySlice, byte[]>
data)
throws IOException {
File sstFile = newSstFile();
- SortLookupStoreWriter writer = storeFactory.createWriter(sstFile,
null);
+ SortLookupStoreWriter writer =
+ storeFactory.createWriter(sstFile,
bloomFilterBuilderFactory.apply(data.size()));
Review Comment:
Could we make the Bloom-filter hash consistent with the configured key
comparator, or avoid enabling the Bloom filter when comparator equality is not
guaranteed to imply byte equality?
`LocalKvDb` uses the configured comparator for MemTable/SST ordering and
lookup, but the Bloom filter hashes the raw serialized key bytes. Thus two keys
that compare equal but have different encodings can be found before flush and
incorrectly rejected after flush.
This occurs with Paimon's own `RowCompactedSerializer` comparator: two FLOAT
NaNs with different payload bits serialize to different bytes, while the slice
comparator returns `0`. I reproduced:
rawKeysEqual=false, comparator=0
beforeFlush=value
afterFlush=null
Disabling the Bloom filter restores the expected result. Please add a
regression test and either provide a comparator-consistent key hasher/canonical
encoding or conservatively disable this optimization where byte equality is not
guaranteed.
##########
paimon-common/src/main/java/org/apache/paimon/lookup/sort/db/LocalKvDb.java:
##########
@@ -302,6 +334,12 @@ public void bulkLoad(Iterator<Map.Entry<byte[], byte[]>>
sortedEntries) throws I
previousFileMaxKey,
targetLevel);
}
+
+ checkArgument(
Review Comment:
Could we clean up all SST files created by `bulkLoad` when validation or
iteration fails?
This validation can run after one or more output files have already been
closed. For example, bulk-loading three entries with `numEntries=4` throws as
expected, but leaves all three SST files in the directory while the database
still reports zero level files:
sstFilesAfterFailure=3
dbLevelFilesAfterFailure=0
The same leak can occur when the iterator or an ordering check fails after
earlier files have been finalized. Please delete `currentSstFile` and every
file recorded in `bulkLoadFiles` before propagating the exception, and extend
the failure tests to verify directory cleanup.
--
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]
