Add binary and string view to integration tests #455

Alex-PLACET · 2025-06-18T07:00:03Z

No description provided.

JohanMabille · 2025-06-20T13:19:35Z

include/sparrow/utils/ranges.hpp

+    constexpr bool operator==(const std::span<T, N1>& lhs, const std::span<T, N2>& rhs)
+    {
+        return std::ranges::equal(lhs, rhs);
+    }


Adding new functions into std namespace is undefined behavior.

Fixed, now the == nullable operator use deep comparison for std::span

JohanMabille · 2025-06-20T13:20:39Z

test/c_data_integration/include/sparrow/c_data_integration/constant.hpp

+    inline constexpr std::string_view SIZE = "SIZE";
+    inline constexpr std::string_view INLINED = "INLINED";
+    inline constexpr std::string_view PREFIX_HEX = "PREFIX_HEX";
+    inline constexpr std::string_view BUFFER_INDEX = "BUFFER_INDEX";


Nitpicking: constexpr implies inline, so inline can be removed here, and from the previous lines too (sorry for not catching it earlier).

Actually I was wrong, constexpr implies inline for functions only, not for variables, sorry for that.

codecov · 2025-06-20T13:28:18Z

Codecov Report

Attention: Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.25%. Comparing base (8ce3116) to head (9f8d160).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/array_factory.cpp	0.00%	2 Missing ⚠️
include/sparrow/layout/dispatch.hpp	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #455      +/-   ##
==========================================
- Coverage   88.29%   88.25%   -0.05%     
==========================================
  Files         100      100              
  Lines        7410     7433      +23     
==========================================
+ Hits         6543     6560      +17     
- Misses        867      873       +6

Flag	Coverage Δ
unittests	`88.25% <66.66%> (-0.05%)`	⬇️

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.

Copilot

Pull Request Overview

This PR adds integration tests for binary and string view handling by introducing new parser implementations and updating associated test and build files.

Added binaryview_parser and utf8view_array_from_json functions to parse binary and utf8 view data.
Removed the old stringview_parser and updated the test JSON file list and build system to support the new view types.

Reviewed Changes

Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test/c_data_integration/test_c_data_integration/test_c_data_integration.cpp	Updated test JSON file paths and ordering to include binary view tests.
test/c_data_integration/src/utils.cpp	Added a new hex_string_to_bytes function and refactored related code.
test/c_data_integration/src/json_parser.cpp	Updated to include binaryview_parser and remapped "utf8view" to utf8view_array_from_json.
test/c_data_integration/src/binaryview_parser.cpp	Added a new parser implementation for binary and utf8 view arrays.
test/c_data_integration/CMakeLists.txt	Updated to include the new binaryview_parser and remove the deprecated stringview_parser.
src/array_factory.cpp	Added support for STRING_VIEW and BINARY_VIEW data types.
Other header files	Updated types, traits, and layout interfaces to support and integrate the new view types.

Comments suppressed due to low confidence (2)

include/sparrow/types/data_type.hpp:681

The removal of std::string_view from all_base_types_extended_t alters the extended type set. Please confirm that this change is deliberate, as it could affect type trait resolutions elsewhere.

    using all_base_types_extended_t = mpl::append_t<all_base_types_t, char>;

test/c_data_integration/test_c_data_integration/test_c_data_integration.cpp:51

There appear to be duplicate JSON file paths from different positions in the list. Please verify that the ordering and duplication of file entries is intentional to avoid redundant test executions.

    json_files_path / "null-trivial.json",

Copilot · 2025-06-24T09:31:40Z

test/c_data_integration/src/binaryview_parser.cpp

+                    view_data.end(),
+                    buffer.begin() + offset,
+                    buffer.begin() + offset + size - static_cast<int>(prefix_bytes.size())
+                );


The calculation for the insertion range might result in an invalid range if prefix_bytes.size() exceeds size. Consider adding a validation check to ensure that 'offset + size' is not less than prefix_bytes.size().

JohanMabille

I just realized the value_type of variable_size_binary_view is either a std::string_view or a sequence_view. I think it should be the same as variable_size_binary_array, that is, std::string or std::vector<std::byte>.std::stirng_viewandsequence_view` should be used for const_reference types only.

JohanMabille · 2025-07-02T07:33:26Z

include/sparrow/types/data_type.hpp

        std::vector<byte_t>,
+        sparrow::sequence_view<const byte_t>,


all_base_types_t should contain types for the inner value_type only, not types used in the const_reference type.,

JohanMabille · 2025-07-02T07:37:53Z

include/sparrow/types/data_traits.hpp

+        static constexpr data_type type_id = data_type::STRING_VIEW;
+        using value_type = std::string_view;
+        using const_reference = std::string_view;
+    };


Same for the traits, they should be defined for types participating in the value_type variant, not types participating in the const_reference variant.

JohanMabille · 2025-07-02T07:43:14Z

include/sparrow/utils/nullable.hpp

+        requires(
+            mpl::is_type_instance_of_v<std::ranges::range_value_t<R>, nullable>
+            && std::is_same_v<typename std::ranges::range_value_t<R>::value_type, T>
+        )


Why this change? nullable_of is easier to read.

JohanMabille · 2025-07-02T07:43:22Z

include/sparrow/utils/nullable.hpp

+        requires(
+            mpl::is_type_instance_of_v<std::ranges::range_value_t<R>, nullable>
+            && std::is_same_v<typename std::ranges::range_value_t<R>::value_type, T>
+        )


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

Alex-PLACET self-assigned this Jun 18, 2025

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

This comment was marked as outdated.

Sign in to view

Alex-PLACET force-pushed the add_binary_view_c_data_structure_integration_tests branch from 63e5ca7 to b6c4daa Compare June 20, 2025 12:37

JohanMabille requested changes Jun 20, 2025

View reviewed changes

Alex-PLACET marked this pull request as ready for review June 20, 2025 13:53

Alex-PLACET force-pushed the add_binary_view_c_data_structure_integration_tests branch from 455f61c to a0934a9 Compare June 20, 2025 13:56

JohanMabille approved these changes Jun 20, 2025

View reviewed changes

Alex-PLACET force-pushed the add_binary_view_c_data_structure_integration_tests branch 3 times, most recently from 950abe1 to 7c690f9 Compare June 24, 2025 07:40

Alex-PLACET requested a review from Copilot June 24, 2025 09:31

Copilot AI reviewed Jun 24, 2025

View reviewed changes

Alex-PLACET force-pushed the add_binary_view_c_data_structure_integration_tests branch 3 times, most recently from d889e9b to 01f150e Compare July 1, 2025 13:23

JohanMabille reviewed Jul 2, 2025

View reviewed changes

Alex-PLACET force-pushed the add_binary_view_c_data_structure_integration_tests branch 2 times, most recently from 77a470e to 434ee7e Compare July 4, 2025 06:50

Add binary and string view to integration tests

afe151a

JohanMabille force-pushed the add_binary_view_c_data_structure_integration_tests branch from bfd6717 to 46f9840 Compare July 4, 2025 12:22

Fixed build

df8d61c

JohanMabille force-pushed the add_binary_view_c_data_structure_integration_tests branch from 46f9840 to df8d61c Compare July 4, 2025 12:46

Added padding to short string data in binary view

92eea8c

JohanMabille force-pushed the add_binary_view_c_data_structure_integration_tests branch 2 times, most recently from 36a2399 to fdd65b4 Compare July 8, 2025 07:45

test

a35f125

JohanMabille force-pushed the add_binary_view_c_data_structure_integration_tests branch from 5c4577a to a35f125 Compare July 8, 2025 07:52

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

9f8d160

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

JohanMabille merged commit 1fce28a into man-group:main Jul 8, 2025
166 of 168 checks passed

Alex-PLACET deleted the add_binary_view_c_data_structure_integration_tests branch July 8, 2025 12:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add binary and string view to integration tests #455

Add binary and string view to integration tests #455

Uh oh!

Alex-PLACET commented Jun 18, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

JohanMabille Jun 20, 2025

Uh oh!

Alex-PLACET Jun 20, 2025

Uh oh!

JohanMabille Jun 20, 2025

Uh oh!

Alex-PLACET Jun 20, 2025

Uh oh!

JohanMabille Jul 1, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jun 20, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jun 24, 2025

Uh oh!

JohanMabille left a comment

Uh oh!

JohanMabille Jul 2, 2025

Uh oh!

JohanMabille Jul 2, 2025

Uh oh!

JohanMabille Jul 2, 2025

Uh oh!

JohanMabille Jul 2, 2025

Uh oh!

Uh oh!

Uh oh!

Add binary and string view to integration tests #455

Add binary and string view to integration tests #455

Uh oh!

Conversation

Alex-PLACET commented Jun 18, 2025

Uh oh!

This comment was marked as outdated.

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!

JohanMabille Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

JohanMabille left a comment

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!

Uh oh!

JohanMabille Jul 1, 2025 •

edited

Loading

codecov bot commented Jun 20, 2025 •

edited

Loading