Skip to content

Conversation

@Wumpf
Copy link
Member

@Wumpf Wumpf commented Aug 18, 2023

What

We now use our own status type everywhere, allowing us to predeclare everything arrow.
On top of that I made sure that we include a much more minimal set of arrow headers for the serialization code itself.

There's a static assertion in the unit test that ensures that we don't start leaking arrow headers into the future

Haven't measured, but it feels like this also improved compile times of the sdk which were already pretty good.


The one thing that's noted in #2873 but is not happening here yet (?) is splitting out the utility methods. The idea would be this (actual generated code I already have on a test branch!) :

        struct LineStrip2D {
// .........
            /// Creates a Rerun DataCell from an array of LineStrip2D components.
            static Result<rerun::DataCell> to_data_cell(
                const LineStrip2D* instances, size_t num_instances
            );
        };

        /// Serialization utilities for arrays of LineStrip2D
        namespace LineStrip2DSerializationUtils {
            /// Returns the arrow data type this type corresponds to.
            static const std::shared_ptr<arrow::DataType>& to_arrow_datatype();

            /// Creates a new array builder with an array of this type.
            static Result<std::shared_ptr<arrow::ListBuilder>> new_arrow_array_builder(
                arrow::MemoryPool* memory_pool
            );

            /// Fills an arrow array builder with an array of this type.
            static Error fill_arrow_array_builder(
                arrow::ListBuilder* builder, const LineStrip2D* elements, size_t num_elements
            );
        } // namespace LineStrip2DSerializationUtils

The problem is that this makes calling these helpers unnecessary complicated sicne I need to append SerializationUtils to the typename of some previously static method calls which is suprisingly cumbersome 🤔
EDIT: Briefly chatted with Emil about it. Let's do it, it's actually not that hard

Checklist

@Wumpf Wumpf added the sdk-cpp C/C++ API specific label Aug 18, 2023
@jleibs jleibs self-requested a review August 18, 2023 15:46
@Wumpf Wumpf force-pushed the andreas/cpp/dont-leak-arrow-headers branch from b0a11aa to 3cac667 Compare August 18, 2023 16:38
@Wumpf Wumpf force-pushed the andreas/cpp/dont-leak-arrow-headers branch from 3cac667 to 41408a4 Compare August 19, 2023 08:41
@Wumpf Wumpf merged commit a55e884 into main Aug 19, 2023
@Wumpf Wumpf deleted the andreas/cpp/dont-leak-arrow-headers branch August 19, 2023 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sdk-cpp C/C++ API specific

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't leak arrow headers into Rerun C++ API headers

3 participants