leaves12138 commented on code in PR #8613:
URL: https://github.com/apache/paimon/pull/8613#discussion_r3576959629
##########
paimon-common/src/main/java/org/apache/paimon/globalindex/bitmap/BitmapGlobalIndexWriter.java:
##########
@@ -76,10 +81,21 @@ public void write(@Nullable Object key, long relativeRowId)
{
}
nonNullRows.add(relativeRowId);
- updateMinMax(key);
- BitmapGlobalIndexFormat.SerializedKey serializedKey =
-
BitmapGlobalIndexFormat.SerializedKey.fromObject(keySerializer, key);
- bitmaps.computeIfAbsent(serializedKey, k -> new
RoaringNavigableMap64()).add(relativeRowId);
+ if (lastKey != null) {
+ int comparison = comparator.compare(key, lastKey);
+ if (comparison < 0) {
Review Comment:
`BitmapGlobalIndexer.createWriter()` is also used by
`GenericIndexTopoBuilder` and Spark's `DefaultGlobalIndexBuilder`; both feed
rows in row-id/data-file order, not index-key order. This new monotonicity
requirement therefore breaks existing bitmap global-index creation for ordinary
unsorted columns (for example, the Spark bitmap test writes `name_9` before
`name_10`, which is descending lexicographically). Sorting the unit-test inputs
hides that production regression. Could we keep the unordered writer for
generic callers and use a separate streaming writer only for the PK-sorted
path, or add an external sort to every production builder?
##########
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:
Once this stream is opened, it is only closed by `finish()`. If row
iteration, validation, or a later write fails, the writer is not `Closeable`,
so the existing Flink/Spark cleanup hooks cannot close it; `PkSortedIndexFile`
also only deletes the path. This can leak the stream or an object-store
multipart upload. Could the writer implement a failure-safe close/abort path,
and could the PK-sorted builder close it before deleting created files?
--
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]
