From 5f18e7b39a22a5f84dd702cd701a8ec46459228d Mon Sep 17 00:00:00 2001 From: AlenkaF Date: Tue, 4 Jun 2024 11:56:00 +0200 Subject: [PATCH 01/11] Update FromArraysInternal --- cpp/src/arrow/array/array_nested.cc | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/cpp/src/arrow/array/array_nested.cc b/cpp/src/arrow/array/array_nested.cc index 2f6bca3d571e..df2bcc64021f 100644 --- a/cpp/src/arrow/array/array_nested.cc +++ b/cpp/src/arrow/array/array_nested.cc @@ -847,14 +847,13 @@ Result> MapArray::FromArraysInternal( const auto& typed_offsets = checked_cast(*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); + null_count = -1; } + buffers[1] = typed_offsets.values(); return std::make_shared(type, offsets->length() - 1, std::move(buffers), keys, items, /*null_count=*/null_count, offsets->offset()); } From 00d363455620de8888b6d738e2eebbe6aab57806 Mon Sep 17 00:00:00 2001 From: AlenkaF Date: Tue, 4 Jun 2024 12:25:12 +0200 Subject: [PATCH 02/11] Fix null_count --- cpp/src/arrow/array/array_list_test.cc | 2 +- cpp/src/arrow/array/array_nested.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/array/array_list_test.cc b/cpp/src/arrow/array/array_list_test.cc index 063b68706b31..e9cf694eb963 100644 --- a/cpp/src/arrow/array/array_list_test.cc +++ b/cpp/src/arrow/array/array_list_test.cc @@ -1374,7 +1374,7 @@ TEST_F(TestMapArray, FromArrays) { offsets3->data()->buffers[0])); ASSERT_OK(map7->Validate()); MapArray expected7(map_type, length, offsets1->data()->buffers[1], keys, items, - offsets3->data()->buffers[0], 1); + offsets3->data()->buffers[0], 3); AssertArraysEqual(expected7, *map7); // Null bitmap and offset with null diff --git a/cpp/src/arrow/array/array_nested.cc b/cpp/src/arrow/array/array_nested.cc index df2bcc64021f..b31c37eed2b3 100644 --- a/cpp/src/arrow/array/array_nested.cc +++ b/cpp/src/arrow/array/array_nested.cc @@ -851,7 +851,7 @@ Result> MapArray::FromArraysInternal( int64_t null_count = 0; if (null_bitmap) { buffers[0] = std::move(null_bitmap); - null_count = -1; + null_count = internal::CountSetBits(null_bitmap->data(), 0, offsets->length()); } buffers[1] = typed_offsets.values(); return std::make_shared(type, offsets->length() - 1, std::move(buffers), keys, From 55f127c8ac5c4b822c358157239df061de73a0b4 Mon Sep 17 00:00:00 2001 From: AlenkaF Date: Tue, 4 Jun 2024 14:59:30 +0200 Subject: [PATCH 03/11] Restore null_count to -1, update tests and make them more readable --- cpp/src/arrow/array/array_list_test.cc | 20 ++++++++++++++++---- cpp/src/arrow/array/array_nested.cc | 2 +- python/pyarrow/tests/test_array.py | 14 ++++++++++++++ 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/cpp/src/arrow/array/array_list_test.cc b/cpp/src/arrow/array/array_list_test.cc index e9cf694eb963..3d18d5f967b7 100644 --- a/cpp/src/arrow/array/array_list_test.cc +++ b/cpp/src/arrow/array/array_list_test.cc @@ -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], 3); + 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])); diff --git a/cpp/src/arrow/array/array_nested.cc b/cpp/src/arrow/array/array_nested.cc index b31c37eed2b3..df2bcc64021f 100644 --- a/cpp/src/arrow/array/array_nested.cc +++ b/cpp/src/arrow/array/array_nested.cc @@ -851,7 +851,7 @@ Result> MapArray::FromArraysInternal( int64_t null_count = 0; if (null_bitmap) { buffers[0] = std::move(null_bitmap); - null_count = internal::CountSetBits(null_bitmap->data(), 0, offsets->length()); + null_count = -1; } buffers[1] = typed_offsets.values(); return std::make_shared(type, offsets->length() - 1, std::move(buffers), keys, diff --git a/python/pyarrow/tests/test_array.py b/python/pyarrow/tests/test_array.py index 88394c77e429..fa3e4f9b463d 100644 --- a/python/pyarrow/tests/test_array.py +++ b/python/pyarrow/tests/test_array.py @@ -1097,6 +1097,7 @@ def test_map_from_arrays(): items.type), mask=pa.array([False, True, False], type=pa.bool_()) ) + assert result.null_count == expected.null_count assert result.equals(expected) # pass in null bitmap without the type @@ -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 == expected.null_count + 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): From 2f20506cfcaded3e1b76bd536e57707423218097 Mon Sep 17 00:00:00 2001 From: AlenkaF Date: Tue, 4 Jun 2024 15:01:40 +0200 Subject: [PATCH 04/11] Make python tests a bit more specific regarding the null_count --- python/pyarrow/tests/test_array.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/pyarrow/tests/test_array.py b/python/pyarrow/tests/test_array.py index fa3e4f9b463d..b358403f5ee3 100644 --- a/python/pyarrow/tests/test_array.py +++ b/python/pyarrow/tests/test_array.py @@ -1097,7 +1097,7 @@ def test_map_from_arrays(): items.type), mask=pa.array([False, True, False], type=pa.bool_()) ) - assert result.null_count == expected.null_count + assert result.null_count == 1 assert result.equals(expected) # pass in null bitmap without the type @@ -1117,7 +1117,7 @@ def test_map_from_arrays(): mask=pa.array([True, True, False], type=pa.bool_()) ) expected = pa.array(pyentries, type=pa.map_(pa.binary(), pa.int32())) - assert result.null_count == expected.null_count + assert result.null_count == 2 assert result.equals(expected) # error if null bitmap and offsets with nulls passed From 1067c035a3a287f7d3da572802cb7a95dc33679a Mon Sep 17 00:00:00 2001 From: AlenkaF Date: Thu, 6 Jun 2024 12:27:03 +0200 Subject: [PATCH 05/11] Change -1 to kUnknownNullCount --- cpp/src/arrow/array/array_nested.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/array/array_nested.cc b/cpp/src/arrow/array/array_nested.cc index df2bcc64021f..9e7a0b7786b7 100644 --- a/cpp/src/arrow/array/array_nested.cc +++ b/cpp/src/arrow/array/array_nested.cc @@ -851,7 +851,7 @@ Result> MapArray::FromArraysInternal( int64_t null_count = 0; if (null_bitmap) { buffers[0] = std::move(null_bitmap); - null_count = -1; + null_count = kUnknownNullCount; } buffers[1] = typed_offsets.values(); return std::make_shared(type, offsets->length() - 1, std::move(buffers), keys, From 6fc5aa3a38826890df6a93e1b9ff24d5014340ab Mon Sep 17 00:00:00 2001 From: AlenkaF Date: Thu, 6 Jun 2024 12:28:16 +0200 Subject: [PATCH 06/11] Change null_count with MayHaveNulls --- cpp/src/arrow/array/array_nested.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/array/array_nested.cc b/cpp/src/arrow/array/array_nested.cc index 9e7a0b7786b7..fb2efe9ae50c 100644 --- a/cpp/src/arrow/array/array_nested.cc +++ b/cpp/src/arrow/array/array_nested.cc @@ -836,7 +836,7 @@ Result> MapArray::FromArraysInternal( return Status::NotImplemented("Null bitmap with offsets slice not supported."); } - if (offsets->null_count() > 0) { + if (offsets->data()->MayHaveNulls()) { ARROW_ASSIGN_OR_RAISE(auto buffers, CleanListOffsets(NULLPTR, *offsets, pool)); return std::make_shared(type, offsets->length() - 1, std::move(buffers), From 1ee19a098573396a4e5312bd356f945c8273c3c8 Mon Sep 17 00:00:00 2001 From: AlenkaF Date: Thu, 6 Jun 2024 13:48:20 +0200 Subject: [PATCH 07/11] Remove const from null_bitmap --- cpp/src/arrow/array/array_nested.cc | 6 +++--- cpp/src/arrow/array/array_nested.h | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cpp/src/arrow/array/array_nested.cc b/cpp/src/arrow/array/array_nested.cc index fb2efe9ae50c..1053153d88bf 100644 --- a/cpp/src/arrow/array/array_nested.cc +++ b/cpp/src/arrow/array/array_nested.cc @@ -807,7 +807,7 @@ MapArray::MapArray(const std::shared_ptr& type, int64_t length, Result> MapArray::FromArraysInternal( std::shared_ptr type, const std::shared_ptr& offsets, const std::shared_ptr& keys, const std::shared_ptr& items, - MemoryPool* pool, const std::shared_ptr& null_bitmap) { + MemoryPool* pool, std::shared_ptr null_bitmap) { using offset_type = typename MapType::offset_type; using OffsetArrowType = typename CTypeTraits::ArrowType; @@ -861,7 +861,7 @@ Result> MapArray::FromArraysInternal( Result> MapArray::FromArrays( const std::shared_ptr& offsets, const std::shared_ptr& keys, const std::shared_ptr& items, MemoryPool* pool, - const std::shared_ptr& null_bitmap) { + std::shared_ptr null_bitmap) { return FromArraysInternal(std::make_shared(keys->type(), items->type()), offsets, keys, items, pool, null_bitmap); } @@ -869,7 +869,7 @@ Result> MapArray::FromArrays( Result> MapArray::FromArrays( std::shared_ptr type, const std::shared_ptr& offsets, const std::shared_ptr& keys, const std::shared_ptr& items, - MemoryPool* pool, const std::shared_ptr& null_bitmap) { + MemoryPool* pool, std::shared_ptr null_bitmap) { if (type->id() != Type::MAP) { return Status::TypeError("Expected map type, got ", type->ToString()); } diff --git a/cpp/src/arrow/array/array_nested.h b/cpp/src/arrow/array/array_nested.h index f96b6bd3b134..a6d4977839ef 100644 --- a/cpp/src/arrow/array/array_nested.h +++ b/cpp/src/arrow/array/array_nested.h @@ -537,13 +537,13 @@ class ARROW_EXPORT MapArray : public ListArray { static Result> FromArrays( const std::shared_ptr& offsets, const std::shared_ptr& keys, const std::shared_ptr& items, MemoryPool* pool = default_memory_pool(), - const std::shared_ptr& null_bitmap = NULLPTR); + std::shared_ptr null_bitmap = NULLPTR); static Result> FromArrays( std::shared_ptr type, const std::shared_ptr& offsets, const std::shared_ptr& keys, const std::shared_ptr& items, MemoryPool* pool = default_memory_pool(), - const std::shared_ptr& null_bitmap = NULLPTR); + std::shared_ptr null_bitmap = NULLPTR); const MapType* map_type() const { return map_type_; } @@ -563,7 +563,7 @@ class ARROW_EXPORT MapArray : public ListArray { static Result> FromArraysInternal( std::shared_ptr type, const std::shared_ptr& offsets, const std::shared_ptr& keys, const std::shared_ptr& items, - MemoryPool* pool, const std::shared_ptr& null_bitmap = NULLPTR); + MemoryPool* pool, std::shared_ptr null_bitmap = NULLPTR); private: const MapType* map_type_; From 1a175eb00a3e244162a2bae0a20adc80e5a07e33 Mon Sep 17 00:00:00 2001 From: AlenkaF Date: Thu, 6 Jun 2024 13:53:17 +0200 Subject: [PATCH 08/11] Fix C++ linter error --- cpp/src/arrow/array/array_nested.cc | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/cpp/src/arrow/array/array_nested.cc b/cpp/src/arrow/array/array_nested.cc index 1053153d88bf..affb576fb30e 100644 --- a/cpp/src/arrow/array/array_nested.cc +++ b/cpp/src/arrow/array/array_nested.cc @@ -858,18 +858,21 @@ Result> MapArray::FromArraysInternal( items, /*null_count=*/null_count, offsets->offset()); } -Result> MapArray::FromArrays( - const std::shared_ptr& offsets, const std::shared_ptr& keys, - const std::shared_ptr& items, MemoryPool* pool, - std::shared_ptr null_bitmap) { +Result> MapArray::FromArrays(const std::shared_ptr& offsets, + const std::shared_ptr& keys, + const std::shared_ptr& items, + MemoryPool* pool, + std::shared_ptr null_bitmap) { return FromArraysInternal(std::make_shared(keys->type(), items->type()), offsets, keys, items, pool, null_bitmap); } -Result> MapArray::FromArrays( - std::shared_ptr type, const std::shared_ptr& offsets, - const std::shared_ptr& keys, const std::shared_ptr& items, - MemoryPool* pool, std::shared_ptr null_bitmap) { +Result> MapArray::FromArrays(std::shared_ptr type, + const std::shared_ptr& offsets, + const std::shared_ptr& keys, + const std::shared_ptr& items, + MemoryPool* pool, + std::shared_ptr null_bitmap) { if (type->id() != Type::MAP) { return Status::TypeError("Expected map type, got ", type->ToString()); } From eacdc359659fbed6a3304e402b2b976c7c55a4dc Mon Sep 17 00:00:00 2001 From: Alenka Frim Date: Thu, 13 Jun 2024 13:37:56 +0200 Subject: [PATCH 09/11] Update cpp/src/arrow/array/array_nested.cc Co-authored-by: Sutou Kouhei --- cpp/src/arrow/array/array_nested.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/array/array_nested.cc b/cpp/src/arrow/array/array_nested.cc index affb576fb30e..0a244bc492ed 100644 --- a/cpp/src/arrow/array/array_nested.cc +++ b/cpp/src/arrow/array/array_nested.cc @@ -864,7 +864,7 @@ Result> MapArray::FromArrays(const std::shared_ptr MemoryPool* pool, std::shared_ptr null_bitmap) { return FromArraysInternal(std::make_shared(keys->type(), items->type()), - offsets, keys, items, pool, null_bitmap); + offsets, keys, items, pool, std::move(null_bitmap)); } Result> MapArray::FromArrays(std::shared_ptr type, From e0b9c3645ed0a80f9b7182c937b0e7683df21357 Mon Sep 17 00:00:00 2001 From: AlenkaF Date: Thu, 13 Jun 2024 13:41:22 +0200 Subject: [PATCH 10/11] Add std::move --- cpp/src/arrow/array/array_nested.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/array/array_nested.cc b/cpp/src/arrow/array/array_nested.cc index 0a244bc492ed..4950c63ddc64 100644 --- a/cpp/src/arrow/array/array_nested.cc +++ b/cpp/src/arrow/array/array_nested.cc @@ -883,7 +883,7 @@ Result> MapArray::FromArrays(std::shared_ptr ty 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( From 9621d54ded0077e76e5d513d864c3e1c31d145a7 Mon Sep 17 00:00:00 2001 From: AlenkaF Date: Thu, 13 Jun 2024 14:09:21 +0200 Subject: [PATCH 11/11] Fix linter error --- cpp/src/arrow/array/array_nested.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cpp/src/arrow/array/array_nested.cc b/cpp/src/arrow/array/array_nested.cc index 4950c63ddc64..47c0fd35829a 100644 --- a/cpp/src/arrow/array/array_nested.cc +++ b/cpp/src/arrow/array/array_nested.cc @@ -883,7 +883,8 @@ Result> MapArray::FromArrays(std::shared_ptr ty if (!map_type.item_type()->Equals(items->type())) { return Status::TypeError("Mismatching map items type"); } - return FromArraysInternal(std::move(type), offsets, keys, items, pool, std::move(null_bitmap)); + return FromArraysInternal(std::move(type), offsets, keys, items, pool, + std::move(null_bitmap)); } Status MapArray::ValidateChildData(