Implemented map layout #450
Conversation
|
|
||
| ArrowArray arr = make_arrow_array( | ||
| static_cast<std::int64_t>(size), // length | ||
| static_cast<int64_t>(null_count), |
There was a problem hiding this comment.
| static_cast<int64_t>(null_count), | |
| static_cast<std::int64_t>(null_count), |
| std::string("+m"), // format | ||
| name, // name | ||
| metadata, // metadata | ||
| std::nullopt, // flags, |
There was a problem hiding this comment.
You should handle https://arrow.apache.org/docs/dev/format/CDataInterface.html#c.ArrowSchema.flags
ARROW_FLAG_MAP_KEYS_SORTED
| 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; |
There was a problem hiding this comment.
| 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; |
|
|
||
| bool map_array::check_keys_sorted() const | ||
| { | ||
| // TODO: implement this |
src/layout/map_layout/map_value.cpp
Outdated
| { | ||
| return val == k; | ||
| } | ||
| else |
There was a problem hiding this comment.
Can we really enter in this case ?
| size_type index = find_index(key); | ||
| if (index == m_index_end) | ||
| { | ||
| throw std::out_of_range("key not found in map"); |
There was a problem hiding this comment.
Should we have the same behavior as std::map and so create the entry instead of raising an exception ?
There was a problem hiding this comment.
This implementation could be used in at()
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We can have a const at() I guess
There was a problem hiding this comment.
Can you check iterators too ?
There was a problem hiding this comment.
Actually It's already done in the generic_consistency_test (and we have a lot of redundancy if other test cases). Or did you have additional tests in mind?
JohanMabille
force-pushed
the
map_array
branch
3 times, most recently
from
ae6aac9 to
90b4617
Compare
JohanMabille
force-pushed
the
map_array
branch
5 times, most recently
from
209f39b to
d0c0fb7
Compare
JohanMabille
force-pushed
the
map_array
branch
3 times, most recently
from
530819b to
58148bb
Compare
JohanMabille
force-pushed
the
map_array
branch
6 times, most recently
from
1712873 to
e45061f
Compare
There was a problem hiding this comment.
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. |
for more information, see https://pre-commit.ci
No description provided.