Implemented map layout #450

JohanMabille · 2025-06-13T09:30:45Z

No description provided.

Alex-PLACET · 2025-06-13T11:56:14Z

include/sparrow/layout/map_layout/map_array.hpp

+
+        ArrowArray arr = make_arrow_array(
+            static_cast<std::int64_t>(size),  // length
+            static_cast<int64_t>(null_count),


Suggested change

static_cast<int64_t>(null_count),

static_cast<std::int64_t>(null_count),

include/sparrow/layout/map_layout/map_array.hpp

Alex-PLACET · 2025-06-13T11:59:52Z

include/sparrow/layout/map_layout/map_array.hpp

+                std::string("+m"),  // format
+                name,               // name
+                metadata,           // metadata
+                std::nullopt,       // flags,


You should handle https://arrow.apache.org/docs/dev/format/CDataInterface.html#c.ArrowSchema.flags
ARROW_FLAG_MAP_KEYS_SORTED

Alex-PLACET · 2025-06-13T12:01:07Z

include/sparrow/layout/map_layout/map_value.hpp

+        bool empty() const noexcept;
+        size_type size() const noexcept;
+
+        const_mapped_reference operator[](const key_type& key) const;
+
+        const_iterator find(const key_type& key) const noexcept;
+
+        const_iterator begin() const;
+        const_iterator cbegin() const;
+
+        const_iterator end() const;
+        const_iterator cend() const;


Suggested change

bool empty() const noexcept;

size_type size() const noexcept;

const_mapped_reference operator[](const key_type& key) const;

const_iterator find(const key_type& key) const noexcept;

const_iterator begin() const;

const_iterator cbegin() const;

const_iterator end() const;

const_iterator cend() const;

[[nodiscard]] bool empty() const noexcept;

[[nodiscard]] size_type size() const noexcept;

[[nodiscard]] const_mapped_reference operator[](const key_type& key) const;

[[nodiscard]]const_iterator find(const key_type& key) const noexcept;

[[nodiscard]] const_iterator begin() const;

[[nodiscard]] const_iterator cbegin() const;

[[nodiscard]] const_iterator end() const;

[[nodiscard]] const_iterator cend() const;

Alex-PLACET · 2025-06-13T12:02:29Z

src/layout/map_layout/map_array.cpp

+
+    bool map_array::check_keys_sorted() const
+    {
+        // TODO: implement this


To do in this PR ?

Alex-PLACET · 2025-06-13T12:04:28Z

src/layout/map_layout/map_value.cpp

+                            {
+                                return val == k;
+                            }
+                            else


Can we really enter in this case ?

Alex-PLACET · 2025-06-13T12:06:30Z

src/layout/map_layout/map_value.cpp

+        size_type index = find_index(key);
+        if (index == m_index_end)
+        {
+            throw std::out_of_range("key not found in map");


Should we have the same behavior as std::map and so create the entry instead of raising an exception ?

This implementation could be used in at()

That means making the map_array mutable, which implies resizing the underlying children arrays. If we decide to do so, it should probably be done in a dedicated PR.

We can have a const at() I guess

This would suggest a vector or array API instead of a map.

I don't understand, there is an const T& at()in the std: https://en.cppreference.com/w/cpp/container/map/at.html

Ah my bad, I missed this one.

test/test_map_array.cpp

Copilot

Pull Request Overview

This PR implements the map layout functionality including new array and value types for maps, along with tests and necessary integrations in the build system and dispatch mechanism.

Introduces map_array and map_value classes with associated unit tests.
Updates CMake and dispatch to integrate map layout functionality.
Adds definitions and traits for map types in header files.

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
test/test_map_array.cpp	Adds unit tests for map_array functionality (contains debug prints).
test/test_list_value.cpp	Updates include directives to add map_value.
test/CMakeLists.txt	Adjusts test targets by commenting out some tests and adding map_array reference.
src/layout/map_layout/map_value.cpp	New implementation of map_value (spelling typo in license header).
src/layout/map_layout/map_array.cpp	New implementation of map_array (spelling typo and a bug in raw_keys_array non-const method).
include/sparrow/types/data_type.hpp	Adds MAP data type support.
include/sparrow/types/data_traits.hpp	Specializes arrow_traits for map_value.
include/sparrow/layout/map_layout/map_value.hpp	New header for map_value definition.
include/sparrow/layout/map_layout/map_array.hpp	New header for map_array with a spelling issue in an alias and proper API prototypes.
include/sparrow/layout/dispatch.hpp	Updates dispatch to handle MAP type.
CMakeLists.txt	Includes new map layout sources in the build.
.github/workflows/windows.yml	Updates test execution commands for Windows builds.

src/layout/map_layout/map_value.cpp

src/layout/map_layout/map_array.cpp

include/sparrow/layout/map_layout/map_array.hpp

test/test_map_array.cpp

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

codecov · 2025-06-19T13:35:47Z

Codecov Report

Attention: Patch coverage is 75.00000% with 50 lines in your changes missing coverage. Please review.

Project coverage is 87.62%. Comparing base (dc18702) to head (dd59f1b).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/layout/map_layout/map_value.cpp	57.35%	29 Missing ⚠️
src/layout/map_layout/map_array.cpp	80.45%	17 Missing ⚠️
include/sparrow/layout/map_layout/map_array.hpp	94.73%	2 Missing ⚠️
include/sparrow/types/data_type.hpp	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #450      +/-   ##
==========================================
- Coverage   88.30%   87.62%   -0.69%     
==========================================
  Files          96      100       +4     
  Lines        7301     7545     +244     
==========================================
+ Hits         6447     6611     +164     
- Misses        854      934      +80

Flag	Coverage Δ
unittests	`87.62% <75.00%> (-0.69%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Alex-PLACET · 2025-06-19T15:02:46Z

src/layout/map_layout/map_array.cpp

+                                 isorted = true;
+                                 for (std::size_t j = index_begin; j + 1 < index_end; ++j)
+                                 {
+                                     isorted = isorted && (ar[j] < ar[j + 1]);


Add an early exit if isorted == false

Alex-PLACET · 2025-06-19T15:03:12Z

src/layout/map_layout/map_array.cpp

+        {
+            std::size_t index_begin = offsets[i];
+            std::size_t index_end = offsets[i + 1];
+            sorted = sorted


Add an early exit if sorted == false

Alex-PLACET · 2025-06-19T15:05:27Z

test/test_map_array.cpp

Can you add a test with std::format ?

for more information, see https://pre-commit.ci

Alex-PLACET reviewed Jun 13, 2025

View reviewed changes

JohanMabille force-pushed the map_array branch 10 times, most recently from 209f39b to d0c0fb7 Compare June 16, 2025 14:03

Alex-PLACET assigned JohanMabille and unassigned JohanMabille Jun 16, 2025

JohanMabille force-pushed the map_array branch 12 times, most recently from ffa29f5 to ac21167 Compare June 18, 2025 12:50

Alex-PLACET requested review from Copilot June 18, 2025 15:47

Copilot AI reviewed Jun 18, 2025

View reviewed changes

JohanMabille force-pushed the map_array branch from a99b9a0 to 7fd8e64 Compare June 18, 2025 16:05

JohanMabille force-pushed the map_array branch from 4029a0b to 7e2a363 Compare June 19, 2025 13:25

JohanMabille requested a review from Alex-PLACET June 19, 2025 13:25

Alex-PLACET requested changes Jun 19, 2025

View reviewed changes

Implemented map layout

a26aa61

JohanMabille force-pushed the map_array branch from 4489389 to a26aa61 Compare June 20, 2025 14:36

[pre-commit.ci] auto fixes from pre-commit.com hooks

dd59f1b

for more information, see https://pre-commit.ci

JohanMabille requested a review from Alex-PLACET June 20, 2025 14:58

	static_cast<int64_t>(null_count),
	static_cast<std::int64_t>(null_count),

Implemented map layout #450

Are you sure you want to change the base?

Implemented map layout #450

Uh oh!

Conversation

JohanMabille commented Jun 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Jun 19, 2025 •

edited

Loading