Arrow array schema proxy #185

Alex-PLACET · 2024-08-30T14:52:38Z

This first PR is for "read only". You can't reallocate buffers, change the size, etc ...

include/sparrow/arrow_array_schema_proxy.hpp

JohanMabille · 2024-09-04T07:52:36Z

include/sparrow/arrow_array_schema_proxy.hpp

+        [[nodiscard]] int64_t n_buffers() const;
+        [[nodiscard]] int64_t n_children() const;
+        [[nodiscard]] std::vector<sparrow::buffer_view<uint8_t>> buffers() const;
+        [[nodiscard]] std::vector<arrow_proxy> children() const;


I think these methods should return const reference to cached variables instead of rebuilding the vector for each call:

dynamic allocating the vector each time we need to read a buffer in an aray implementation class will be costly

this will guarantee immutability (notice we could fix the current implementation by returning a const std::vector, but we would still have the allocation issue).

JohanMabille · 2024-09-04T07:56:07Z

include/sparrow/arrow_array_schema_proxy.hpp

+    public:
+
+        explicit arrow_proxy(ArrowArray&& array, ArrowSchema&& schema);
+        explicit arrow_proxy(ArrowArray* array, ArrowSchema* schema);


There are situations where we want to reuse the same schema structure for different Array structures, so I think the followinf constructor would also make sense arrao_proxy(ArrowArray&&, ArrowSchema*)

In the case the schema or array members are pointers, we should not call the release callback, that's right ?

JohanMabille · 2024-09-04T07:58:30Z

include/sparrow/arrow_array_schema_proxy.hpp

+        [[nodiscard]] int64_t n_children() const;
+        [[nodiscard]] std::vector<sparrow::buffer_view<uint8_t>> buffers() const;
+        [[nodiscard]] std::vector<arrow_proxy> children() const;
+        [[nodiscard]] std::optional<arrow_proxy> dictionary() const;


Should we provide an additional bitmap method that returns a dynamic_bitset_view (cached)? This would simpify the implementation of the array implementation classes.

In that case, the first buffer_view in the buffer vector should still be the bitmap to avoid confusion (contrary to what has been done in the array_data).

There is no bitmap for Sparse and dense unions, map, and run encoded format.
In that case, we can return an optional<dynamic_bitset>,
You can check the get_buffer_types_from_data_type to see what are the buffers for each format.

In that case it might be better to handle that in the array implementation classes. We can probably factorize out the creation of the bitmap view in an intermediate class rather than using an std::optional for something known at build time.

JohanMabille · 2024-09-04T08:01:10Z

include/sparrow/arrow_array_schema_proxy.hpp

+    overloaded(Ts...) -> overloaded<Ts...>;
+
+    template <typename T>
+    [[nodiscard]] T& get_variant(auto& var)


The name is confusing, and this function should probably be a private method or array_proxy.

Klaim · 2024-09-04T14:19:24Z

include/sparrow/arrow_array_schema_proxy.hpp

+    void arrow_proxy::initialize_buffers()
+    {
+        m_buffers.clear();
+        const auto buffer_count = static_cast<size_t>(array().n_buffers);


Note that this will fail in 32bit, I'll fix it with the 32bit fix by replacing static_cast by a custom one.

include/sparrow/arrow_array_schema_proxy.hpp

Klaim · 2024-09-04T14:33:04Z

include/sparrow/arrow_array_schema_proxy.hpp

+            ArrowArray& array = std::get<1>(m_array);
+            if (array.release != nullptr)
+            {
+                array.release(&array);


If this throws , abort will be called (because we are in a destructor which is implicitly noexcept(true)). Same issue with schema's release. Maybe wrap these calls around a "safe invoke"? Something like we did in mamba: https://github.com/mamba-org/mamba/blob/main/libmamba/include/mamba/core/invoke.hpp#L12
Not sure what to do if it fails though, but I suppose at a minimum both release must be called, one failing shouldnt prevent the call to the second one.

If we want to crash anyway, ignore this.

Klaim · 2024-09-04T14:33:50Z

include/sparrow/arrow_array_schema_proxy.hpp

+
+    arrow_proxy::~arrow_proxy()
+    {
+        if (m_array.index() == 1)


Not a fan of the duplication here, although it could be refactored later.

include/sparrow/arrow_array_schema_proxy.hpp

include/sparrow/arrow_interface/arrow_array_schema_proxy_utils.hpp

Klaim · 2024-09-04T14:38:58Z

include/sparrow/arrow_interface/arrow_array_schema_proxy_utils.hpp

+        SIZES_64BIT,
+    };
+
+    constexpr std::vector<buffer_type> get_buffer_types_from_data_type(data_type data_type)


Not sure what a "buffer type" is here, so noob question: doest it have to be a vector here? (maybe a todo not to replace by a "small vector" later would be nice here?)

buffer_type is an enum which is just above.
We just return an ordered container of enum. So yes it could be improved

I saw the enum but it is not clear to me exactly what the values means etc. Should be fixed once documented 👍🏼

include/sparrow/arrow_interface/arrow_flag_utils.hpp

include/sparrow/utils/variant_visitor.hpp

Klaim

LGTM, only suggested tweaks to the documentation. The arrow_proxy type also needs to be better clarified as to what to expect of it's behavior, but once it's done it's good for me 👍🏽

Klaim · 2024-09-05T14:59:31Z

include/sparrow/arrow_interface/arrow_array_schema_proxy_utils.hpp

@@ -19,12 +19,14 @@

 namespace sparrow
 {
+    /// Validates the number of buffers in an ArrowArray for a given data type.


Suggested change

/// Validates the number of buffers in an ArrowArray for a given data type.

/// @returns `true` if the number of buffers in an `ArrowArray` for a given data type is valid, `false` otherwise.

Klaim · 2024-09-05T14:59:51Z

include/sparrow/arrow_interface/arrow_array_schema_proxy_utils.hpp

    constexpr bool validate_buffers_count(data_type data_type, int64_t n_buffers)
    {
        const std::size_t expected_buffer_count = get_expected_buffer_count(data_type);
        return static_cast<std::size_t>(n_buffers) == expected_buffer_count;
    }

+    /// Get the expected number of children for a given data type.


Suggested change

/// Get the expected number of children for a given data type.

/// @returns The expected number of children for a given data type.

Klaim · 2024-09-05T15:00:30Z

include/sparrow/arrow_interface/arrow_array_schema_proxy_utils.hpp

@@ -64,6 +66,7 @@ namespace sparrow
        mpl::unreachable();
    }

+    /// Validates the format of an ArrowArray for a given data type.


Suggested change

/// Validates the format of an ArrowArray for a given data type.

/// @returns `true` if the format of an `ArrowArray` for a given data type is valid, `false` otherwise.

Klaim · 2024-09-05T15:01:21Z

include/sparrow/arrow_interface/arrow_array_schema_proxy_utils.hpp

@@ -84,6 +87,8 @@ namespace sparrow
        SIZES_64BIT,
    };

+    /// Provide a vector of buffer types for a given data type.


Suggested change

/// Provide a vector of buffer types for a given data type.

/// @returns A vector of buffer types for a given data type.

Klaim · 2024-09-05T15:03:19Z

include/sparrow/arrow_interface/arrow_array_schema_proxy_utils.hpp

@@ -130,6 +135,7 @@ namespace sparrow
        mpl::unreachable();
    }

+    /// Get The expected offset element count for a given data type and array length.


Suggested change

/// Get The expected offset element count for a given data type and array length.

/// @returns The expected offset element count for a given data type and array length.

Klaim · 2024-09-05T15:03:49Z

include/sparrow/arrow_interface/arrow_flag_utils.hpp

@@ -16,6 +16,7 @@

 namespace sparrow
 {
+    /// Check that the given value is a valid ArrowFlag value.


Suggested change

/// Check that the given value is a valid ArrowFlag value.

/// @returns `true` if the given value is a valid `ArrowFlag` value, `false` otherwise.

Klaim · 2024-09-05T15:04:23Z

include/sparrow/arrow_interface/arrow_flag_utils.hpp

@@ -32,6 +33,7 @@ namespace sparrow
        );
    }

+    /// Convert a bitfield of ArrowFlag values to a vector of ArrowFlag values.


Suggested change

/// Convert a bitfield of ArrowFlag values to a vector of ArrowFlag values.

/// Converts a bitfield of `ArrowFlag` values to a vector of `ArrowFlag` values.

Klaim · 2024-09-05T15:04:52Z

include/sparrow/arrow_interface/arrow_flag_utils.hpp

@@ -52,6 +54,7 @@ namespace sparrow
        return flags;
    }

+    /// Convert a vector of ArrowFlag values to a bitfield of ArrowFlag values.


Suggested change

/// Convert a vector of ArrowFlag values to a bitfield of ArrowFlag values.

/// Converts a vector of `ArrowFlag` values to a bitfield of `ArrowFlag` values.

Klaim · 2024-09-05T15:09:46Z

Clarifications: the issue in the destructor should be fixed (or explicitly stated in the code as intently ignored) before merging this.

JohanMabille · 2024-09-06T09:22:24Z

The conan failure is unrelated to this PR, let's merge and fix it in a dedicated PR.

Alex-PLACET force-pushed the arrow_array_schema_proxy branch from c1dbdcd to 4b6cf82 Compare September 3, 2024 09:03

Alex-PLACET commented Sep 3, 2024

View reviewed changes

include/sparrow/arrow_array_schema_proxy.hpp Outdated Show resolved Hide resolved

Alex-PLACET marked this pull request as ready for review September 3, 2024 09:24

Alex-PLACET requested a review from JohanMabille September 3, 2024 12:17

JohanMabille reviewed Sep 4, 2024

View reviewed changes

Alex-PLACET requested a review from JohanMabille September 4, 2024 12:11

JohanMabille approved these changes Sep 4, 2024

View reviewed changes

Alex-PLACET added 7 commits September 4, 2024 16:05

Add Arrow Array and Schema Proxy

730f7a8

Fix

02f0966

Improve tests

5e1d7f8

wip

bb766d9

Dedicated files for utils

9e6ce9b

Improvements

5ccad34

Add documentation

79e0218

Alex-PLACET force-pushed the arrow_array_schema_proxy branch from 6af9ffa to 79e0218 Compare September 4, 2024 14:07

Add documentation

767de2b

Klaim reviewed Sep 4, 2024

View reviewed changes

Alex-PLACET added 4 commits September 5, 2024 08:52

WIP

38bca9c

Add documentation

f283e68

Add documentation

96550df

Fix

974c04c

Klaim approved these changes Sep 5, 2024

View reviewed changes

Alex-PLACET added 2 commits September 6, 2024 09:31

Address issues

9701f89

Try fix

c6ed176

JohanMabille merged commit 702fc0a into man-group:main Sep 6, 2024
55 of 56 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arrow array schema proxy #185

Arrow array schema proxy #185

Alex-PLACET commented Aug 30, 2024 •

edited

Loading

JohanMabille Sep 4, 2024

Alex-PLACET Sep 4, 2024

JohanMabille Sep 4, 2024

Alex-PLACET Sep 4, 2024 •

edited

Loading

JohanMabille Sep 4, 2024

JohanMabille Sep 4, 2024

Alex-PLACET Sep 4, 2024

JohanMabille Sep 4, 2024

JohanMabille Sep 4, 2024

Alex-PLACET Sep 5, 2024

Klaim Sep 4, 2024

Klaim Sep 4, 2024

Klaim Sep 4, 2024

Klaim Sep 4, 2024

Alex-PLACET Sep 4, 2024

Klaim Sep 4, 2024

Klaim left a comment

Klaim Sep 5, 2024

Klaim Sep 5, 2024

Klaim Sep 5, 2024

Klaim Sep 5, 2024

Klaim Sep 5, 2024

Klaim Sep 5, 2024

Klaim Sep 5, 2024

Klaim Sep 5, 2024

Klaim commented Sep 5, 2024

JohanMabille commented Sep 6, 2024

	/// Validates the number of buffers in an ArrowArray for a given data type.
	/// @returns `true` if the number of buffers in an `ArrowArray` for a given data type is valid, `false` otherwise.

	/// Get the expected number of children for a given data type.
	/// @returns The expected number of children for a given data type.

	/// Validates the format of an ArrowArray for a given data type.
	/// @returns `true` if the format of an `ArrowArray` for a given data type is valid, `false` otherwise.

	/// Provide a vector of buffer types for a given data type.
	/// @returns A vector of buffer types for a given data type.

	/// Get The expected offset element count for a given data type and array length.
	/// @returns The expected offset element count for a given data type and array length.

	/// Check that the given value is a valid ArrowFlag value.
	/// @returns `true` if the given value is a valid `ArrowFlag` value, `false` otherwise.

	/// Convert a bitfield of ArrowFlag values to a vector of ArrowFlag values.
	/// Converts a bitfield of `ArrowFlag` values to a vector of `ArrowFlag` values.

	/// Convert a vector of ArrowFlag values to a bitfield of ArrowFlag values.
	/// Converts a vector of `ArrowFlag` values to a bitfield of `ArrowFlag` values.

Arrow array schema proxy #185

Arrow array schema proxy #185

Conversation

Alex-PLACET commented Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alex-PLACET Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Klaim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Klaim commented Sep 5, 2024

JohanMabille commented Sep 6, 2024

Alex-PLACET commented Aug 30, 2024 •

edited

Loading

Alex-PLACET Sep 4, 2024 •

edited

Loading