Skip to content

GH-41955: [C++] Follow up of adding null_bitmap to MapArray::FromArrays#41956

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
felipecrv merged 11 commits into from
Jun 13, 2024
Merged

GH-41955: [C++] Follow up of adding null_bitmap to MapArray::FromArrays #41956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5f18e7b
Update FromArraysInternal
AlenkaF Jun 4, 2024
00d3634
Fix null_count
AlenkaF Jun 4, 2024
55f127c
Restore null_count to -1, update tests and make them more readable
AlenkaF Jun 4, 2024
2f20506
Make python tests a bit more specific regarding the null_count
AlenkaF Jun 4, 2024
1067c03
Change -1 to kUnknownNullCount
AlenkaF Jun 6, 2024
6fc5aa3
Change null_count with MayHaveNulls
AlenkaF Jun 6, 2024
1ee19a0
Remove const from null_bitmap
AlenkaF Jun 6, 2024
1a175eb
Fix C++ linter error
AlenkaF Jun 6, 2024
eacdc35
Update cpp/src/arrow/array/array_nested.cc
AlenkaF Jun 13, 2024
e0b9c36
Add std::move
AlenkaF Jun 13, 2024
9621d54
Fix linter error
AlenkaF Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions cpp/src/arrow/array/array_list_test.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1369,14 +1369,26 @@ TEST_F(TestMapArray, FromArrays) {
ASSERT_RAISES(Invalid,
MapArray::FromArrays(offsets1, keys_with_null, tmp_items, pool_));

// With null_bitmap
ASSERT_OK_AND_ASSIGN(auto map7, MapArray::FromArrays(offsets1, keys, items, pool_,
offsets3->data()->buffers[0]));
// With null_bitmap and null_count=1
auto null_bitmap_1 = ArrayFromJSON(boolean(), "[1, 0, 1]")->data()->buffers[1];
ASSERT_OK_AND_ASSIGN(auto map7,
MapArray::FromArrays(offsets1, keys, items, pool_, null_bitmap_1));
ASSERT_OK(map7->Validate());
MapArray expected7(map_type, length, offsets1->data()->buffers[1], keys, items,
offsets3->data()->buffers[0], 1);
null_bitmap_1, 1);
ASSERT_EQ(map7->null_count(), 1);
AssertArraysEqual(expected7, *map7);

// With null_bitmap and null_count=2
auto null_bitmap_2 = ArrayFromJSON(boolean(), "[0, 1, 0]")->data()->buffers[1];
ASSERT_OK_AND_ASSIGN(auto map8,
MapArray::FromArrays(offsets1, keys, items, pool_, null_bitmap_2));
ASSERT_OK(map8->Validate());
MapArray expected8(map_type, length, offsets1->data()->buffers[1], keys, items,
null_bitmap_2, 2);
ASSERT_EQ(map8->null_count(), 2);
AssertArraysEqual(expected8, *map8);

// Null bitmap and offset with null
ASSERT_RAISES(Invalid, MapArray::FromArrays(offsets3, keys, items, pool_,
offsets3->data()->buffers[0]));
Expand Down
41 changes: 22 additions & 19 deletions cpp/src/arrow/array/array_nested.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -807,7 +807,7 @@ MapArray::MapArray(const std::shared_ptr<DataType>& type, int64_t length,
Result<std::shared_ptr<Array>> MapArray::FromArraysInternal(
std::shared_ptr<DataType> type, const std::shared_ptr<Array>& offsets,
const std::shared_ptr<Array>& keys, const std::shared_ptr<Array>& items,
MemoryPool* pool, const std::shared_ptr<Buffer>& null_bitmap) {
MemoryPool* pool, std::shared_ptr<Buffer> null_bitmap) {
using offset_type = typename MapType::offset_type;
using OffsetArrowType = typename CTypeTraits<offset_type>::ArrowType;

Expand DownExpand Up@@ -836,7 +836,7 @@ Result<std::shared_ptr<Array>> MapArray::FromArraysInternal(
return Status::NotImplemented("Null bitmap with offsets slice not supported.");
}

if (offsets->null_count() > 0) {
if (offsets->data()->MayHaveNulls()) {
Comment thread
AlenkaF marked this conversation as resolved.
Outdated
ARROW_ASSIGN_OR_RAISE(auto buffers,
CleanListOffsets<MapType>(NULLPTR, *offsets, pool));
return std::make_shared<MapArray>(type, offsets->length() - 1, std::move(buffers),
Expand All@@ -847,30 +847,32 @@ Result<std::shared_ptr<Array>> MapArray::FromArraysInternal(
const auto& typed_offsets = checked_cast<const OffsetArrayType&>(*offsets);

BufferVector buffers;
int64_t null_count;
if (null_bitmap != nullptr) {
buffers = BufferVector({std::move(null_bitmap), typed_offsets.values()});
null_count = null_bitmap->size();
} else {
buffers = BufferVector({null_bitmap, typed_offsets.values()});
null_count = 0;
buffers.resize(2);
int64_t null_count = 0;
if (null_bitmap) {
buffers[0] = std::move(null_bitmap);
Comment thread
AlenkaF marked this conversation as resolved.
Outdated
null_count = kUnknownNullCount;
}
buffers[1] = typed_offsets.values();
return std::make_shared<MapArray>(type, offsets->length() - 1, std::move(buffers), keys,
items, /*null_count=*/null_count, offsets->offset());
}

Result<std::shared_ptr<Array>> MapArray::FromArrays(
const std::shared_ptr<Array>& offsets, const std::shared_ptr<Array>& keys,
const std::shared_ptr<Array>& items, MemoryPool* pool,
const std::shared_ptr<Buffer>& null_bitmap) {
Result<std::shared_ptr<Array>> MapArray::FromArrays(const std::shared_ptr<Array>& offsets,
const std::shared_ptr<Array>& keys,
const std::shared_ptr<Array>& items,
MemoryPool* pool,
std::shared_ptr<Buffer> null_bitmap) {
return FromArraysInternal(std::make_shared<MapType>(keys->type(), items->type()),
offsets, keys, items, pool, null_bitmap);
offsets, keys, items, pool, std::move(null_bitmap));
}

Result<std::shared_ptr<Array>> MapArray::FromArrays(
std::shared_ptr<DataType> type, const std::shared_ptr<Array>& offsets,
const std::shared_ptr<Array>& keys, const std::shared_ptr<Array>& items,
MemoryPool* pool, const std::shared_ptr<Buffer>& null_bitmap) {
Result<std::shared_ptr<Array>> MapArray::FromArrays(std::shared_ptr<DataType> type,
const std::shared_ptr<Array>& offsets,
const std::shared_ptr<Array>& keys,
const std::shared_ptr<Array>& items,
MemoryPool* pool,
std::shared_ptr<Buffer> null_bitmap) {
Comment thread
AlenkaF marked this conversation as resolved.
Outdated
if (type->id() != Type::MAP) {
return Status::TypeError("Expected map type, got ", type->ToString());
}
Expand All@@ -881,7 +883,8 @@ Result<std::shared_ptr<Array>> MapArray::FromArrays(
if (!map_type.item_type()->Equals(items->type())) {
return Status::TypeError("Mismatching map items type");
}
return FromArraysInternal(std::move(type), offsets, keys, items, pool, null_bitmap);
return FromArraysInternal(std::move(type), offsets, keys, items, pool,
std::move(null_bitmap));
}

Status MapArray::ValidateChildData(
Expand Down
6 changes: 3 additions & 3 deletions cpp/src/arrow/array/array_nested.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -537,13 +537,13 @@ class ARROW_EXPORT MapArray : public ListArray {
static Result<std::shared_ptr<Array>> FromArrays(
const std::shared_ptr<Array>& offsets, const std::shared_ptr<Array>& keys,
const std::shared_ptr<Array>& items, MemoryPool* pool = default_memory_pool(),
const std::shared_ptr<Buffer>& null_bitmap = NULLPTR);
std::shared_ptr<Buffer> null_bitmap = NULLPTR);

static Result<std::shared_ptr<Array>> FromArrays(
std::shared_ptr<DataType> type, const std::shared_ptr<Array>& offsets,
const std::shared_ptr<Array>& keys, const std::shared_ptr<Array>& items,
MemoryPool* pool = default_memory_pool(),
const std::shared_ptr<Buffer>& null_bitmap = NULLPTR);
std::shared_ptr<Buffer> null_bitmap = NULLPTR);

const MapType* map_type() const { return map_type_; }

Expand All@@ -563,7 +563,7 @@ class ARROW_EXPORT MapArray : public ListArray {
static Result<std::shared_ptr<Array>> FromArraysInternal(
std::shared_ptr<DataType> type, const std::shared_ptr<Array>& offsets,
const std::shared_ptr<Array>& keys, const std::shared_ptr<Array>& items,
MemoryPool* pool, const std::shared_ptr<Buffer>& null_bitmap = NULLPTR);
MemoryPool* pool, std::shared_ptr<Buffer> null_bitmap = NULLPTR);

private:
const MapType* map_type_;
Expand Down
14 changes: 14 additions & 0 deletions python/pyarrow/tests/test_array.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1097,6 +1097,7 @@ def test_map_from_arrays():
items.type),
mask=pa.array([False, True, False], type=pa.bool_())
)
assert result.null_count == 1
assert result.equals(expected)

# pass in null bitmap without the type
Expand All@@ -1106,6 +1107,19 @@ def test_map_from_arrays():
)
assert result.equals(expected)

# pass in null bitmap with two nulls
offsets = [0, None, None, 6]
pyentries = [None, None, pypairs[2:]]

result = pa.MapArray.from_arrays([0, 2, 2, 6], keys, items, pa.map_(
keys.type,
items.type),
mask=pa.array([True, True, False], type=pa.bool_())
)
expected = pa.array(pyentries, type=pa.map_(pa.binary(), pa.int32()))
assert result.null_count == 2
assert result.equals(expected)

# error if null bitmap and offsets with nulls passed
msg1 = 'Ambiguous to specify both validity map and offsets with nulls'
with pytest.raises(pa.ArrowInvalid, match=msg1):
Expand Down