Add copy trackers in tests #651

Alex-PLACET · 2026-02-04T15:24:44Z

No description provided.

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

…PLACET/sparrow into add_copy_trackers_in_tests

codecov · 2026-02-04T15:37:34Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.67%. Comparing base (e7ed35d) to head (a136355).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #651      +/-   ##
==========================================
+ Coverage   88.30%   88.67%   +0.36%     
==========================================
  Files         112      112              
  Lines        8857     9048     +191     
==========================================
+ Hits         7821     8023     +202     
+ Misses       1036     1025      -11

Flag	Coverage Δ
unittests	`88.67% <100.00%> (+0.36%)`	⬆️

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.

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

Copilot

Pull request overview

This PR introduces copy-tracking instrumentation across several array/buffer types and updates the test suite to assert expected copy/move semantics when SPARROW_TRACK_COPIES is enabled (via the TRACK_COPIES CMake option).

Changes:

Added copy_tracker::reset/count assertions to many copy/move-related tests (guarded by #ifdef SPARROW_TRACK_COPIES).
Instrumented copy constructors / copy paths in multiple core types (arrays, Arrow C-interface helpers, buffers) to call copy_tracker::increase(...).
Enabled TRACK_COPIES=ON in CI workflows so copy-tracking assertions run in CI.

Reviewed changes

Copilot reviewed 41 out of 41 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
test/test_variable_size_binary_view_array.cpp	Adds copy-tracking assertions for copy construction.
test/test_union_array.cpp	Adds copy-tracking assertions for dense/sparse union array copies.
test/test_timestamp_array.cpp	Adds copy/move tracking assertions and refactors move test to compare moved result.
test/test_struct_array.cpp	Adds copy-tracking assertions for struct_array copies.
test/test_string_array.cpp	Adds tracking for constructor/copy/move semantics of string/variable-size binary arrays.
test/test_run_end_encoded_array.cpp	Adds copy-tracking assertions for run_end_encoded_array copies.
test/test_record_batch.cpp	Switches to type-based copy-tracker key for record_batch tests.
test/test_primitive_array.cpp	Adds copy-tracking assertions around ArrowArray/buffer copies and a few constructors.
test/test_null_array.cpp	Adds copy-tracking assertions for null_array copies.
test/test_list_array.cpp	Adds copy-tracking assertions for list/list_view/fixed_sized_list array copies.
test/test_fixed_width_binary_array.cpp	Adds copy/move tracking assertions for fixed_width_binary_array.
test/test_dictionary_encoded_array.cpp	Adds copy-tracking assertions for dictionary_encoded_array copies.
test/test_decimal_array.cpp	Adds tracking to constructors and introduces new copy/move test cases.
src/union_array.cpp	Adds keys and copy tracking increments for dense/sparse union array copies.
src/struct_array.cpp	Adds key and copy tracking increment for struct_array copy ctor.
src/run_end_encoded_array.cpp	Adds key and copy tracking increment for run_end_encoded_array copy ctor.
src/record_batch.cpp	Minor include cleanup (keeps copy tracker include).
src/null_array.cpp	Adds key and copy tracking increment for null_array copy ctor.
src/map_array.cpp	Adds key and copy tracking increment for map_array copy ctor.
src/arrow_interface/arrow_schema.cpp	Increments copy-tracker when deep-copying ArrowSchema.
src/arrow_interface/arrow_array.cpp	Increments copy-tracker when deep-copying ArrowArray.
include/sparrow/variable_size_binary_view_array.hpp	Adds copy-tracker integration + explicit copy ctor to track copies.
include/sparrow/variable_size_binary_array.hpp	Adds copy-tracker integration + explicit copy ctor to track copies.
include/sparrow/union_array.hpp	Declares sparse_union_array copy ctor/assignment for tracking.
include/sparrow/u8_buffer.hpp	Adds copy-tracker key + increments on u8_buffer copy ctor.
include/sparrow/timestamp_array.hpp	Adds copy-tracker key overload + increments on timestamp_array copy ctor.
include/sparrow/primitive_array.hpp	Removes redundant `<cstdint>` include (now pulled elsewhere).
include/sparrow/null_array.hpp	Declares null_array copy ctor/assignment for tracking.
include/sparrow/list_array.hpp	Adds copy-tracker keys + tracks list/list_view/fixed_sized_list copies.
include/sparrow/layout/primitive_array_impl.hpp	Adds copy-tracker key overload + increments on primitive_array_impl copy ctor.
include/sparrow/fixed_width_binary_array.hpp	Adds key + explicit copy ctor to track fixed_width_binary array copies.
include/sparrow/dictionary_encoded_array.hpp	Adds copy-tracker key overload + increments on dictionary_encoded_array copy ctor.
include/sparrow/decimal_array.hpp	Adds copy-tracker key overload + increments on decimal_array copy ctor.
include/sparrow/buffer/buffer_view.hpp	Adds copy tracking to buffer_view copy ctor.
include/sparrow/buffer/buffer.hpp	Adds copy-tracker key + increments on buffer copy ctors.
include/sparrow/arrow_interface/arrow_schema.hpp	Adds key for ArrowSchema to support schema copy tracking.
include/sparrow/arrow_interface/arrow_array.hpp	Adds key for ArrowArray to support array copy tracking.
.github/workflows/qemu.yaml	Enables `-DTRACK_COPIES=ON` in CI.
.github/workflows/osx.yml	Enables `-DTRACK_COPIES=ON` in CI.
.github/workflows/manylinux.yml	Enables `-DTRACK_COPIES=ON` in CI.
.github/workflows/linux.yml	Enables `-DTRACK_COPIES=ON` in CI.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-06T13:31:25Z

include/sparrow/u8_buffer.hpp

+    namespace copy_tracker
+    {
+        template <typename T>
+        std::string key_u8_buffer()
+        {
+            return "u8_buffer<" + std::string(typeid(T).name()) + ">";
+        }


key_u8_buffer() uses typeid(T).name() but this header doesn't include <typeinfo>. Please include <typeinfo> explicitly (or remove the RTTI dependency) to avoid build fragility across toolchains/stdlibs.

test/test_decimal_array.cpp

Copilot · 2026-02-06T13:31:26Z

test/test_string_array.cpp

+#ifdef SPARROW_TRACK_COPIES
+                CHECK_EQ(copy_tracker::count("variable_size_binary_array"), 0);
+#endif


This check hard-codes the key string ("variable_size_binary_array") even though the test already resets via copy_tracker::key<layout_type>(). Using the same key<layout_type>() for count() avoids drift if the key string ever changes and keeps the test consistent.

Copilot · 2026-02-06T13:31:26Z

test/test_string_array.cpp

+                layout_type arr{
+                    std::move(data_buffer),
+                    std::move(offset_buffer),
+                    where_nulls,
+                    "name",
+                    metadata_sample_opt
+                };


layout_type arr{...}; is only used for its side effects (construction) and then never referenced. If the test build treats -Wunused-variable as an error, this will fail. Consider using CHECK_NOTHROW(...) again, or explicitly mark it used (e.g., (void)arr;).

include/sparrow/decimal_array.hpp

Copilot · 2026-02-06T13:31:27Z

include/sparrow/buffer/buffer_view.hpp

+    {
+        copy_tracker::increase(copy_tracker::key_buffer_view<T>());


buffer_view is intended to be a cheap, non-owning view, but the new copy ctor calls copy_tracker::increase(copy_tracker::key_buffer_view<T>()), which constructs a std::string (allocations) and can throw. This changes the copy from trivially/noexcept copyable to potentially-throwing and adds overhead even when copy tracking is disabled (the key string is still built). Consider making this tracking compile-time gated (e.g., #ifdef SPARROW_TRACK_COPIES / if constexpr (copy_tracker::is_enabled())) and/or using a non-allocating key type (e.g., std::string_view / const char*) so copying a view stays noexcept and cheap.

Suggested change

{

copy_tracker::increase(copy_tracker::key_buffer_view<T>());

{

#if defined(SPARROW_TRACK_COPIES)

copy_tracker::increase(copy_tracker::key_buffer_view<T>());

#endif

Copilot · 2026-02-06T13:31:27Z

include/sparrow/buffer/buffer.hpp

+    namespace copy_tracker
+    {
+        template <typename T>
+        std::string key_buffer()
+        {
+            return "buffer<" + std::string(typeid(T).name()) + ">";
+        }


key_buffer() uses typeid(T).name() but this header doesn't include <typeinfo>. Relying on transitive includes for std::type_info/typeid is fragile and can break with different standard library implementations. Add an explicit #include <typeinfo> (or avoid typeid here).

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

JohanMabille

We should also track copies in T& T::operator=(const T&).

JohanMabille · 2026-02-09T09:42:15Z

include/sparrow/buffer/buffer.hpp

            this->create_storage(rhs.size());
            get_data().p_end = copy_initialize(rhs.begin(), rhs.end(), get_data().p_begin, get_allocator());
        }
+        copy_tracker::increase(copy_tracker::key_buffer<T>());


You should also inscrease copies number in operator=.

JohanMabille · 2026-02-09T09:43:41Z

include/sparrow/buffer/buffer_view.hpp

+        : p_data(other.p_data)
+        , m_size(other.m_size)
+    {
+        copy_tracker::increase(copy_tracker::key_buffer_view<T>());


You should also increase copies number in operator=

JohanMabille · 2026-02-09T09:44:46Z

include/sparrow/buffer/buffer.hpp

+    namespace copy_tracker
+    {
+        template <typename T>
+        std::string key_buffer()


What about keeping the generic name key and constrain the template, like you did for primitive_array_impl ?

JohanMabille · 2026-02-09T09:45:36Z

include/sparrow/buffer/buffer_view.hpp

+    namespace copy_tracker
+    {
+        template <class T>
+        std::string key_buffer_view()


What about keeping the generic name key and constrain the template like you did for primitive_array_impl ?

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

…PLACET/sparrow into add_copy_trackers_in_tests

JohanMabille

I think we're missing a last copy tracker in dictionary_encoded_array copy assign operator.

include/sparrow/dictionary_encoded_array.hpp

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

Alex-PLACET and others added 6 commits February 4, 2026 13:16

Add copy trackers for tests

3c295c3

wip

6544ec7

Add more copy trackers tests

4599a05

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

bade9c9

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

Activate copy trackers in github workflows

c671b9a

Merge branch 'add_copy_trackers_in_tests' of https://github.com/Alex-…

f180352

…PLACET/sparrow into add_copy_trackers_in_tests

Alex-PLACET requested a review from JohanMabille February 4, 2026 16:46

Alex-PLACET self-assigned this Feb 4, 2026

Alex-PLACET marked this pull request as ready for review February 4, 2026 16:46

Alex-PLACET and others added 2 commits February 5, 2026 09:52

wip

6e015c9

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

3246b41

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

Alex-PLACET requested a review from Copilot February 6, 2026 13:23

Copilot started reviewing on behalf of Alex-PLACET February 6, 2026 13:23 View session

Copilot AI reviewed Feb 6, 2026

View reviewed changes

Alex-PLACET and others added 2 commits February 9, 2026 09:05

Update test/test_decimal_array.cpp

3eeb83c

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update include/sparrow/decimal_array.hpp

46e8ac1

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

JohanMabille requested changes Feb 9, 2026

View reviewed changes

Alex-PLACET and others added 5 commits February 9, 2026 13:27

Add copy trackers on operator=

9ab0734

wip

86aaf71

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

820b3af

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

wip

63fa9ab

Merge branch 'add_copy_trackers_in_tests' of https://github.com/Alex-…

0529bfb

…PLACET/sparrow into add_copy_trackers_in_tests

Alex-PLACET requested a review from JohanMabille February 9, 2026 12:59

JohanMabille reviewed Feb 9, 2026

View reviewed changes

include/sparrow/dictionary_encoded_array.hpp Show resolved Hide resolved

fix dictionary

7cc985d

JohanMabille approved these changes Feb 9, 2026

View reviewed changes

Alex-PLACET and others added 2 commits February 9, 2026 15:19

wip

57dc3b8

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

a136355

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

Alex-PLACET merged commit 0401405 into man-group:main Feb 9, 2026
157 of 158 checks passed

Alex-PLACET deleted the add_copy_trackers_in_tests branch February 9, 2026 14:43

		{
		copy_tracker::increase(copy_tracker::key_buffer_view<T>());

Add copy trackers in tests #651

Add copy trackers in tests #651

Uh oh!

Conversation

Alex-PLACET commented Feb 4, 2026

Uh oh!

codecov bot commented Feb 4, 2026 • 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 Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Feb 4, 2026 •

edited

Loading