Add integration tests #61

Alex-PLACET · 2025-11-10T14:06:05Z

The point of the PR is to be able to run the integration test with archery and be able to run the tests on the primitive.json without any issue.
You will see that the workflow is red, it's because of the other layouts.
The plan is to fix the other layout in next PRs.

codecov-commenter · 2025-11-10T14:51:54Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@93c2fe8). Learn more about missing BASE report.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #61   +/-   ##
=======================================
  Coverage        ?   82.31%           
=======================================
  Files           ?       35           
  Lines           ?     1742           
  Branches        ?        0           
=======================================
  Hits            ?     1434           
  Misses          ?      308           
  Partials        ?        0

Flag	Coverage Δ
unittests	`82.31% <100.00%> (?)`

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 comprehensive integration tests for Arrow IPC file format serialization and deserialization, including tools for converting between JSON, stream, and file formats. The implementation properly tracks record batch blocks in the footer to enable random access to batches within Arrow files.

Key Changes

Implements footer block tracking with offset, metadata_length, and body_length for each record batch
Adds nullable flag handling in deserialization to properly preserve schema information
Creates integration testing tools compatible with Apache Arrow's Archery framework
Adds Docker-based integration testing workflow

Reviewed changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
include/sparrow_ipc/serialize.hpp	Adds serialized_record_batch_info struct and updates serialize_record_batch to return block metadata
src/serialize.cpp	Implements block info calculation with proper 8-byte alignment per Arrow spec
include/sparrow_ipc/stream_file_serializer.hpp	Adds record_batch_block struct and tracking vector for footer generation
src/stream_file_serializer.cpp	Updates write_footer to populate footer with tracked record batch blocks
src/deserialize.cpp	Adds nullable flag parameter to deserialization functions
include/sparrow_ipc/deserialize_*.hpp	Updates function signatures to accept nullable parameter
src/deserialize_fixedsizebinary_array.cpp	Implements nullable flag handling in schema construction
integration_tests/*	New integration test infrastructure with JSON/stream/file conversion tools
tests/test_stream_file_serializer.cpp	Adds extensive footer validation tests
.github/workflows/integration_tests.yaml	New CI workflow for Docker-based integration testing
CMakeLists.txt	Adds SPARROW_IPC_BUILD_INTEGRATION_TESTS option and output directory configuration

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

integration_tests/test_integration_tools.cpp

integration_tests/src/integration_tools.cpp

integration_tests/arrow_validate.cpp

ci/docker/integration.dockerfile

include/sparrow_ipc/serialize.hpp

integration_tests/test_integration_tools.cpp

integration_tests/arrow_stream_to_file.cpp

integration_tests/CMakeLists.txt

integration_tests/test_integration_tools.cpp

Copilot

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.

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

tests/test_stream_file_serializer.cpp

integration_tests/src/integration_tools.cpp

ci/docker/integration.dockerfile

Copilot

Pull request overview

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

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

Copilot · 2025-12-01T13:09:06Z

tests/test_stream_file_serializer.cpp

+    const org::apache::arrow::flatbuf::Footer* get_footer_from_file_data(const std::vector<uint8_t>& file_data)
+    {
+        // Footer size is stored 4 bytes before the trailing magic
+        const size_t footer_size_offset = file_data.size() - sparrow_ipc::arrow_file_magic_size - sizeof(int32_t);
+        int32_t footer_size = 0;
+        std::memcpy(&footer_size, file_data.data() + footer_size_offset, sizeof(int32_t));
+
+        // Footer data starts at footer_size_offset - footer_size
+        const size_t footer_offset = footer_size_offset - footer_size;
+
+        return org::apache::arrow::flatbuf::GetFooter(file_data.data() + footer_offset);
+    }


This helper function get_footer_from_file_data is duplicated in both tests/test_stream_file_serializer.cpp (line 270) and integration_tests/test_integration_tools.cpp (line 24). Consider extracting this into a shared test utility header (e.g., tests/include/test_helpers.hpp) to avoid code duplication and make maintenance easier.

Hind-M · 2025-12-02T14:02:37Z

.github/copilot-instructions.md

+1. Schema mismatches in stream → `std::invalid_argument`
+2. Deallocating source buffer while arrays in use → undefined behavior
+3. Missing RPATH on Linux → runtime linking errors
+4. Only LZ4 compression supported (not ZSTD yet)


Both are supported now.

Hind-M · 2025-12-02T14:02:59Z

.github/workflows/integration_tests.yaml

+      - name: Install specific version of tzdata
+        run: sudo apt-get install tzdata


I don't think we need this?

Hind-M · 2025-12-02T14:03:14Z

.github/workflows/integration_tests.yaml

+      - name: List all folders and subfolders
+        run: |
+          echo "Listing all folders and subfolders:"
+          find . -type d


Is this a leftover from debugging?

Yes, I will delete that

Hind-M · 2025-12-02T14:03:37Z

ci/docker/integration.dockerfile

+# Clone the arrow monorepo // TODO: change to the official repo
+RUN git clone --depth 1 --branch archery_supports_external_libraries https://github.com/Alex-PLACET/arrow.git /arrow-integration --recurse-submodules


Is there a reason why we're not using the official repo already?

Yes, the official repo don't support external library to be tested

Hind-M · 2025-12-02T14:07:58Z

src/serialize.cpp

@@ -1,18 +1,22 @@
+#include "sparrow_ipc/serialize.hpp"


Including project headers should be done after c++ standard ones (i.e <cstdint> and <optional>).

This is not the current format rules

Hind-M · 2025-12-02T14:23:24Z

include/sparrow_ipc/stream_file_serializer.hpp

+     *
+     * Each block describes the location and size of a record batch in the file.
+     */
+    struct record_batch_block


record_batch_block name isn't really explicit regarding what it should be, maybe rename to something else to clarify? record_batch_metadata or record_batch_info or record_batch_offsets (preference for the latter)...

It refers to this: https://github.com/apache/arrow/blob/main/format/File.fbs#L39

Hind-M · 2025-12-02T15:27:29Z

integration_tests/test_integration_tools.cpp

+#include "sparrow_ipc/stream_file_serializer.hpp"
+
+// Helper function to extract and parse the footer from Arrow IPC file data
+const org::apache::arrow::flatbuf::Footer* get_footer_from_file_data(const std::vector<uint8_t>& file_data)


As this file (test_integration_tools.cpp) is testing a local one (integration_tools.cpp), we may want to move it to the global tests folder?
That way we could also move this helper function to the dedicated sparrow_ipc_tests_helpers.hpp file and avoid duplicated code in the same project.

Hind-M · 2025-12-02T15:49:53Z

The integration tests job seems to be still failing?

EDIT: Ok, it's the other layouts I suppose?

Hind-M · 2025-12-02T16:05:06Z

Maybe I missed it but are we actually running test_integration_tools in the CI?

This reverts commit 280467d.

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

Alex-PLACET self-assigned this Nov 10, 2025

Alex-PLACET force-pushed the add_integration_tests branch from 265a553 to 02539e5 Compare November 24, 2025 07:52

Alex-PLACET requested a review from Copilot December 1, 2025 10:15

Copilot started reviewing on behalf of Alex-PLACET December 1, 2025 10:15 View session

Copilot finished reviewing on behalf of Alex-PLACET December 1, 2025 10:19

Copilot AI reviewed Dec 1, 2025

View reviewed changes

Alex-PLACET requested a review from Copilot December 1, 2025 10:29

Copilot started reviewing on behalf of Alex-PLACET December 1, 2025 10:30 View session

Copilot finished reviewing on behalf of Alex-PLACET December 1, 2025 10:32

Copilot AI reviewed Dec 1, 2025

View reviewed changes

tests/test_stream_file_serializer.cpp Outdated Show resolved Hide resolved

integration_tests/src/integration_tools.cpp Outdated Show resolved Hide resolved

ci/docker/integration.dockerfile Show resolved Hide resolved

Alex-PLACET requested a review from Copilot December 1, 2025 13:05

Copilot started reviewing on behalf of Alex-PLACET December 1, 2025 13:06 View session

Copilot finished reviewing on behalf of Alex-PLACET December 1, 2025 13:08

Copilot AI reviewed Dec 1, 2025

View reviewed changes

Alex-PLACET marked this pull request as ready for review December 1, 2025 13:44

Alex-PLACET marked this pull request as draft December 1, 2025 15:52

Alex-PLACET force-pushed the add_integration_tests branch from 87b0774 to 204eb30 Compare December 2, 2025 09:14

Alex-PLACET requested review from Hind-M and JohanMabille December 2, 2025 09:15

Alex-PLACET marked this pull request as ready for review December 2, 2025 10:04

Hind-M reviewed Dec 2, 2025

View reviewed changes

Alex-PLACET added 6 commits December 3, 2025 11:08

Add integration tests

677c30d

try fix

8b95f4d

fix

f005312

fix

481f766

fix

2a037da

wip

9381762

Alex-PLACET and others added 18 commits December 3, 2025 11:09

wip

235d29b

Revert "wip"

8cf8396

This reverts commit 280467d.

fix

dd6b9cd

wip

32403c9

wip

15d2e46

wip

86fdaeb

wip

e2b4796

wip

f0164de

wip

b72850e

wip

ec41430

Add tests

aae02cc

wip

a80b678

Apply suggestions from code review

e204b75

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

wip

2498182

wip

5af59a2

fix metadata size

3dd44ec

try fix

f30c862

wip

85569fd

Alex-PLACET force-pushed the add_integration_tests branch from 204eb30 to 85569fd Compare December 3, 2025 10:52

Alex-PLACET added 7 commits December 3, 2025 17:01

Fix according review

010112a

fix

41fa566

try remove

e5f3492

Fix

fd49249

fix

69af240

Fix

beb2666

get_footer_from_file_data move to flatbuffer_utils

67f10c1

Alex-PLACET requested a review from Hind-M December 4, 2025 09:04

Hind-M approved these changes Dec 4, 2025

View reviewed changes

Alex-PLACET merged commit b00c770 into QuantStack:main Dec 5, 2025
26 of 28 checks passed

Alex-PLACET deleted the add_integration_tests branch December 5, 2025 09:17

		- name: Install specific version of tzdata
		run: sudo apt-get install tzdata

		# Clone the arrow monorepo // TODO: change to the official repo
		RUN git clone --depth 1 --branch archery_supports_external_libraries https://github.com/Alex-PLACET/arrow.git /arrow-integration --recurse-submodules

Add integration tests #61

Add integration tests #61

Uh oh!

Conversation

Alex-PLACET commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Nov 10, 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

Key Changes

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 1, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hind-M Dec 2, 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!

Choose a reason for hiding this comment

Uh oh!

Hind-M commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hind-M commented Dec 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Alex-PLACET commented Nov 10, 2025 •

edited

Loading

codecov-commenter commented Nov 10, 2025 •

edited

Loading

Hind-M Dec 2, 2025 •

edited

Loading

Hind-M commented Dec 2, 2025 •

edited

Loading