leaves12138 commented on code in PR #8613:
URL: https://github.com/apache/paimon/pull/8613#discussion_r3577468057
##########
paimon-common/src/main/java/org/apache/paimon/globalindex/bitmap/BitmapIndexReader.java:
##########
@@ -346,26 +344,27 @@ private RoaringNavigableMap64 scanSerializedDictionary(
private int firstPossibleDictionaryBlock(
List<BitmapGlobalIndexFormat.DictionaryBlockMeta> blocks,
BitmapGlobalIndexFormat.SerializedKey key) {
- int index = findDictionaryBlockIndex(blocks, key);
+ int index = findSerializedDictionaryBlockIndex(blocks, key);
return Math.max(index, 0);
}
- private BitmapGlobalIndexFormat.BlockInfo findBitmapBlock(
- BitmapGlobalIndexFormat.SerializedKey key) {
+ private BitmapGlobalIndexFormat.BlockInfo findBitmapBlock(Object key) {
List<BitmapGlobalIndexFormat.DictionaryBlockMeta> blocks =
dictionaryBlocks.get();
if (blocks.isEmpty()) {
return null;
}
- int index = findDictionaryBlockIndex(blocks, key);
+ int index = findLogicalDictionaryBlockIndex(blocks, key);
Review Comment:
Existing version-1 bitmap files were written with dictionary blocks sorted
by serialized key bytes, while this lookup now assumes logical key order. The
new streaming writer also still writes `VERSION = 1`, so old and new files with
different ordering semantics are indistinguishable. For numeric keys this can
return false negatives; for example, an old v1 file with separate blocks for
`0` and `-1` is ordered as `[0, -1]` by serialized bytes, but this binary
search assumes `[-1, 0]`. Could we retain the version-1 serialized-order read
path and write the new logical-order format with a new version, as in the
earlier implementation?
##########
paimon-common/src/main/java/org/apache/paimon/globalindex/bitmap/BitmapGlobalIndexWriter.java:
##########
@@ -110,12 +121,30 @@ public List<ResultEntry> finish() {
return Collections.singletonList(new ResultEntry(fileName, rowCount,
meta));
}
- private void updateMinMax(Object key) {
- if (firstKey == null || comparator.compare(key, firstKey) < 0) {
- firstKey = key;
+ private void flushCurrentBitmap() {
+ if (currentBitmap.isEmpty()) {
+ return;
+ }
+ try {
+ streamingWriter()
+ .write(
+ BitmapGlobalIndexFormat.SerializedKey.fromObject(
+ keySerializer, lastKey),
+ currentBitmap);
+ currentBitmap.clear();
+ } catch (IOException e) {
+ throw new RuntimeException("Error in writing bitmap index files.",
e);
}
- if (lastKey == null || comparator.compare(key, lastKey) > 0) {
- lastKey = key;
+ }
+
+ private BitmapGlobalIndexFormat.StreamingWriter streamingWriter() throws
IOException {
+ if (streamingWriter == null) {
+ fileName =
fileWriter.newFileName(BitmapGlobalIndexerFactory.IDENTIFIER);
+ outputStream = fileWriter.newOutputStream(fileName);
Review Comment:
The PK-sorted path is covered now, but the sorted Flink/Spark paths still do
not propagate this cleanup. Spark wraps the writer in
`SortedSingleColumnIndexWriter`, which is not closeable and leaves
`currentWriter` open if iteration, `write`, or `finish` fails. Flink's
`WriteIndexOperator` similarly keeps `currentWriter` without closing it from an
operator failure/close path. Could both paths close the active writer from a
failure-safe `finally`/lifecycle hook?
--
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]
