Handle different data types #8

Hind-M · 2025-07-28T13:16:25Z

No description provided.

Hind-M · 2025-07-30T12:20:41Z

tests/test_primitive_array.cpp

+        REQUIRE_NE(arrow_array_deserialized_ar, nullptr);
+        compare_arrow_arrays(*arrow_array_ar, *arrow_array_deserialized_ar);
+
+//         compare_values<T>(ar, deserialized_ar);


This is failing on windows, as CHECK_EQ(ar, deserialized_ar);.
cf. error here.
We should either fix this upstream in sparrow (if indeed something is missing) or at least try and use comparison functions from sparrow (to be exposed if not already).

Note that it wasn't the case before this PR, i.e CHECK_EQ(ar, deserialized_ar); was not failing on windows when building (tests are commented out there for now), but maybe this was some latent issue that was exposed when adding more tests and changing the structure?...

This looks like a compiler issue, this operator is defined ins sparrow

NOTE:
We should test again - with this PR merged - using sparrow built from source (and then directly from conda-forge after releasing next version and confirming this fixes the issue).

include/serialize.hpp

JohanMabille · 2025-08-01T09:24:25Z

src/serialize.cpp

+        // - Correctly populating the Flatbuffer-defined metadata for both messages.
+
+        // Create a mutable copy of the input array to allow moving its internal structures
+        sparrow::primitive_array<T> mutable_arr = arr;


Should we also provide a destructive version of serialize to avoid this copy? I.e. something like

template <typename T> std::vector<uint8_t> serialize_primitive_array(sparrow::primitive_array<T>&& arr)

Actually I decided to use extract_arrow_structures before handling the metadata (which we need to get using the primitive_array input argument through arr.metadata() and not through arrow_schema to get the right format).

If we add the overload you suggested, we would need to deep copy the metadata, and that's not ideal.
Instead I'm using now get_arrow_structures to avoid copying...
But I had to remove the const from the primitive_array input argument because of errors due to get_arrow_structures not taking a const.

=> Should we provide an overload for const in sparrow for get_arrow_structures knowing that it's calling get_arrow_proxy, and that can take either a const or a non-const ARRAY?

Alex-PLACET · 2025-08-19T08:05:49Z

include/serialize.hpp

+        {
+            // Create a new builder for the Schema message's metadata
+            flatbuffers::FlatBufferBuilder schema_builder;
+
+            // Create the Field metadata, which describes a single column (or array)
+            flatbuffers::Offset<flatbuffers::String> fb_name_offset = 0;
+            if (arrow_schema.name)
+            {
+                fb_name_offset = schema_builder.CreateString(arrow_schema.name);
+            }
+
+            // Determine the Flatbuffer type information from the C schema's format string
+            auto [type_enum, type_offset] = utils::get_flatbuffer_type(schema_builder, arrow_schema.format);
+
+            // Handle metadata
+            flatbuffers::Offset<flatbuffers::Vector<flatbuffers::Offset<org::apache::arrow::flatbuf::KeyValue>>>
+                fb_metadata_offset = 0;
+
+            if (arr.metadata())
+            {
+                sparrow::key_value_view metadata_view = *(arr.metadata());
+                std::vector<flatbuffers::Offset<org::apache::arrow::flatbuf::KeyValue>> kv_offsets;
+
+                auto mv_it = metadata_view.cbegin();
+                for (auto i = 0; i < metadata_view.size(); ++i, ++mv_it)
+                {
+                    auto key_offset = schema_builder.CreateString(std::string((*mv_it).first));
+                    auto value_offset = schema_builder.CreateString(std::string((*mv_it).second));
+                    kv_offsets.push_back(
+                        org::apache::arrow::flatbuf::CreateKeyValue(schema_builder, key_offset, value_offset));
+                }
+                fb_metadata_offset = schema_builder.CreateVector(kv_offsets);
+            }


Can you put this in a dedicated function ?

Yes there is some more work on #10 which is based on this PR and where there is more refactoring.

Alex-PLACET · 2025-08-19T08:10:22Z

include/serialize.hpp

+    sparrow::primitive_array<T> deserialize_primitive_array(const std::vector<uint8_t>& buffer);
+
+    template <typename T>
+    std::vector<uint8_t> serialize_primitive_array(sparrow::primitive_array<T>& arr)


Suggested change

std::vector<uint8_t> serialize_primitive_array(sparrow::primitive_array<T>& arr)

std::vector<uint8_t> serialize_primitive_array(const sparrow::primitive_array<T>& arr)

This is actually not possible for now when using get_arrow_structures, cf. this comment.

Alex-PLACET · 2025-08-19T08:10:52Z

include/serialize.hpp

+        auto [arrow_arr_ptr, arrow_schema_ptr] = sparrow::get_arrow_structures(arr);
+        auto& arrow_arr = *arrow_arr_ptr;
+        auto& arrow_schema = *arrow_schema_ptr;


Suggested change

auto [arrow_arr_ptr, arrow_schema_ptr] = sparrow::get_arrow_structures(arr);

auto& arrow_arr = *arrow_arr_ptr;

auto& arrow_schema = *arrow_schema_ptr;

const auto [arrow_arr_ptr, arrow_schema_ptr] = sparrow::get_arrow_structures(arr);

const ArrowArray& arrow_arr = *arrow_arr_ptr;

const ArrowSchema& arrow_schema = *arrow_schema_ptr;

Alex-PLACET · 2025-08-19T08:11:23Z

include/serialize.hpp

+        // This will be the final buffer holding the complete IPC stream.
+        std::vector<uint8_t> final_buffer;
+


Move it to l.114 please

Alex-PLACET · 2025-08-19T08:11:45Z

include/serialize.hpp

+            }
+
+            // Determine the Flatbuffer type information from the C schema's format string
+            auto [type_enum, type_offset] = utils::get_flatbuffer_type(schema_builder, arrow_schema.format);


Suggested change

auto [type_enum, type_offset] = utils::get_flatbuffer_type(schema_builder, arrow_schema.format);

const auto [type_enum, type_offset] = utils::get_flatbuffer_type(schema_builder, arrow_schema.format);

Alex-PLACET · 2025-08-19T08:16:11Z

include/serialize.hpp

+            auto fb_fields = schema_builder.CreateVector(fields_vec);
+
+            // Build the Schema object from the vector of fields
+            auto schema_offset = org::apache::arrow::flatbuf::CreateSchema(schema_builder, org::apache::arrow::flatbuf::Endianness::Little, fb_fields);


Alex-PLACET · 2025-08-19T08:16:53Z

include/serialize.hpp

+            schema_builder.Finish(schema_message_offset);
+
+            // Assemble the Schema message bytes
+            uint32_t schema_len = schema_builder.GetSize(); // Get the size of the serialized metadata


Suggested change

uint32_t schema_len = schema_builder.GetSize(); // Get the size of the serialized metadata

const uint32_t schema_len = schema_builder.GetSize(); // Get the size of the serialized metadata

Alex-PLACET · 2025-08-19T08:17:33Z

include/serialize.hpp

+            int64_t validity_size = (arrow_arr.length + 7) / 8;
+            int64_t data_size = arrow_arr.length * sizeof(T);
+            int64_t body_len = validity_size + data_size; // The total size of the message body


Suggested change

int64_t validity_size = (arrow_arr.length + 7) / 8;

int64_t data_size = arrow_arr.length * sizeof(T);

int64_t body_len = validity_size + data_size; // The total size of the message body

const int64_t validity_size = (arrow_arr.length + 7) / 8;

const int64_t data_size = arrow_arr.length * sizeof(T);

const int64_t body_len = validity_size + data_size; // The total size of the message body

Alex-PLACET · 2025-08-19T08:31:08Z

include/serialize.hpp

Can you improve the constness of the variables ?
Can you also split the functions: like for the schema, it's common to all the types

Can you improve the constness of the variables ? Can you also split the functions: like for the schema, it's common to all the types

Yes, see #10 (further improvements will be done there as well).

Alex-PLACET · 2025-08-19T08:38:11Z

tests/test_utils.cpp

+        flatbuffers::FlatBufferBuilder builder;
+        SUBCASE("Null and Boolean types")
+        {
+            CHECK_EQ(utils::get_flatbuffer_type(builder, "n").first, org::apache::arrow::flatbuf::Type::Null);


Please use sparrow::data_type_to_format to get the type in string format.

There are still missing data types in sparrow::data_type_to_format in sparrow actually.
I changed the ones available for now.

Hind-M added 6 commits July 28, 2025 15:15

Handle different data types

610259f

Move utils fcts to utils files

6a737f5

Add namespaces

1c4eeba

Add tests

9ae4709

Rename test.cpp

368c76f

Comment possible offending line

cc8065d

Hind-M force-pushed the format_fct branch from 579ee0f to cc8065d Compare July 29, 2025 15:34

Hind-M added 5 commits July 30, 2025 12:16

Change way of testing

6613f78

Factorize

ccc4313

Comment offending line

451a198

Use same comparion for other test cases

7860dc7

Add comment and use sp namespace

44d2552

Hind-M commented Jul 30, 2025

View reviewed changes

Hind-M marked this pull request as ready for review July 30, 2025 12:23

JohanMabille requested changes Aug 1, 2025

View reviewed changes

Hind-M added 3 commits August 1, 2025 16:55

Move everything to serialize.hpp

a137693

Remove leftover

121a70b

Use get_arrow_structures instead of extract_arrow_structures

4b66da4

Hind-M mentioned this pull request Aug 7, 2025

Serialize/Deserialize null_array #10

Merged

Hind-M added 2 commits August 7, 2025 12:08

Remove leftovers

478d903

Use string instead of string_view

6d55743

Alex-PLACET requested changes Aug 19, 2025

View reviewed changes

Use data_type_to_format in get_flatbuffer_type test

9e8b3fe

Alex-PLACET approved these changes Aug 19, 2025

View reviewed changes

Hind-M merged commit f7dd61b into QuantStack:main Aug 19, 2025
12 checks passed

Hind-M deleted the format_fct branch August 19, 2025 12:07

Hind-M mentioned this pull request Aug 21, 2025

Change way of testing after fix on sparrow #12

Draft

	std::vector<uint8_t> serialize_primitive_array(sparrow::primitive_array<T>& arr)
	std::vector<uint8_t> serialize_primitive_array(const sparrow::primitive_array<T>& arr)

		// This will be the final buffer holding the complete IPC stream.
		std::vector<uint8_t> final_buffer;

	auto [type_enum, type_offset] = utils::get_flatbuffer_type(schema_builder, arrow_schema.format);
	const auto [type_enum, type_offset] = utils::get_flatbuffer_type(schema_builder, arrow_schema.format);

	uint32_t schema_len = schema_builder.GetSize(); // Get the size of the serialized metadata
	const uint32_t schema_len = schema_builder.GetSize(); // Get the size of the serialized metadata

Handle different data types #8

Handle different data types #8

Uh oh!

Conversation

Hind-M commented Jul 28, 2025

Uh oh!

Hind-M Jul 30, 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 Aug 4, 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!

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!

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!

Hind-M Jul 30, 2025 •

edited

Loading

Hind-M Aug 4, 2025 •

edited

Loading