Conversation
wgtmac
force-pushed
the
bitmap
branch
2 times, most recently
from
11497ae to
72b274a
Compare
| [[nodiscard]] Status AddRange(int64_t pos_start, int64_t pos_end); | ||
|
|
||
| /// \brief Checks if a position is set in the bitmap. | ||
| /// \return Result<bool> or InvalidArgument error |
There was a problem hiding this comment.
Invalid argument is for an out of range value?
There was a problem hiding this comment.
We haven't added OutOfRange error kind and so does Arrow C++. This is consistent with the Java impl below:
private static void validatePosition(long pos) {
Preconditions.checkArgument(
pos >= 0 && pos <= MAX_POSITION,
"Bitmap supports positions that are >= 0 and <= %s: %s",
MAX_POSITION,
pos);
}There was a problem hiding this comment.
I was asking more about what types of errors could be thrown, InvalidArgument seems fine (or even just having this be infallible and require requestors to provide a value > 1 and false for anything more then the current bitmap).
| } | ||
| bitmap.ForEach([&](int64_t pos) { | ||
| ASSERT_TRUE(positions.count(pos) > 0) << "Unexpected position: " << pos; | ||
| }); |
There was a problem hiding this comment.
should this also assert empty vs is_empty?
There was a problem hiding this comment.
did you intend to add a check here?
There was a problem hiding this comment.
We don't need this because ASSERT_EQ(bitmap.Cardinality(), positions.size()); above has already checked it.
src/iceberg/deletes/roaring_position_bitmap.cc:196:25 [modernize-use-integer-sign-comparison] Check warning: src/iceberg/deletes/roaring_position_bitmap.cc:196:25 [modernize-use-integer-sign-comparison] comparison between 'signed' and 'unsigned' integers
|
Thanks @emkornfield for your detailed review! I think I've addressed all your comments. Please let me know what you think. |
| /// significant 4 bytes. For each key, a 32-bit Roaring bitmap is | ||
| /// maintained to store positions for that key. | ||
| /// | ||
| /// \note The Puffin deletion-vector-v1 wrapping (length prefix, magic |
There was a problem hiding this comment.
| /// \note The Puffin deletion-vector-v1 wrapping (length prefix, magic | |
| /// \note This class is used to represent deletion vectors. The Puffin puffin reader/write handle adding the additional required framing (length prefix, magic, magic bytes, CRC-32) for `deletion-vector-v1` persistence. |
3 participants
No description provided.