New C++ api#8651
Conversation
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | chunked_varbinview_canonical_into[(1000, 10)] |
154.7 µs | 190.5 µs | -18.8% |
| ⚡ | Simulation | chunked_varbinview_into_canonical[(1000, 10)] |
206.1 µs | 170.1 µs | +21.18% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing myrrc/cxx2 (46cf4f5) with develop (aff0bd3)
Footnotes
-
4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
myrrc
force-pushed
the
myrrc/cxx2
branch
2 times, most recently
from
05babc3 to
5795c45
Compare
myrrc
force-pushed
the
myrrc/cxx2
branch
2 times, most recently
from
a044015 to
a5c7567
Compare
myrrc
force-pushed
the
myrrc/cxx2
branch
7 times, most recently
from
6b2036e to
66bfb31
Compare
| }; | ||
|
|
||
| // 16-bit float | ||
| struct f16 { |
There was a problem hiding this comment.
I guess we could detect at build time what C++ version the compiler supports and branch on using the C++23 std lib f16 or our custom one.
|
|
||
| // Types that a PrimitiveView can hold | ||
| template <class T> | ||
| concept view_type = primitive_type<T> || std::is_same_v<T, bool>; |
There was a problem hiding this comment.
primitive_view? view_type in isolation reads vague.
| * Estimated count. Throws if not known(). For inexact estimates this is | ||
| * is an upper bound. | ||
| */ | ||
| uint64_t value() const; |
There was a problem hiding this comment.
Not sure I understand this API. Why not just have value? What's the flow when the caller passes in the fallback. And why does that need to handled in a method of Estimate?
|
|
||
| // List accessors. Valid only on List and FixedSizeList dtypes | ||
|
|
||
| DataType list_element() const; |
There was a problem hiding this comment.
Every public fn in this PR added needs docs.
|
|
||
| namespace dtype { | ||
|
|
||
| inline constexpr bool Nullable = true; |
There was a problem hiding this comment.
What's the naming convention we wanna go for? Nullable reads like a type. Google convention is kNullable, using the k prefix for constants.
|
|
||
| private: | ||
| void reset() noexcept { | ||
| if (!path_.empty()) { |
There was a problem hiding this comment.
Do we care about checking the error in tests?
| return BytesView(std::move(canonical), std::move(validity), n); | ||
| } | ||
|
|
||
| Array make_struct( |
There was a problem hiding this comment.
Why actually not && in the signature for the return type?
| } | ||
|
|
||
| void StructArrayBuilder::Deleter::operator()( | ||
| vx_struct_column_builder* ptr) const noexcept { |
There was a problem hiding this comment.
What's our rule of thumb for sprinkling noexcept?
| bits_ = other.bits_; | ||
| bit_offset_ = other.bit_offset_; | ||
| all_invalid_ = other.all_invalid_; | ||
| other.owner_ = nullptr; |
There was a problem hiding this comment.
Why is = nullptr responsibility of the assignment operator?
| } | ||
| session_ = std::move(other.session_); | ||
| stream_ = other.stream_; | ||
| other.stream_ = ArrowArrayStream{}; |
There was a problem hiding this comment.
Also here we do why change other? other.stream_ = ArrowArrayStream{};
| #include <utility> | ||
| #include <vector> | ||
|
|
||
| /** |
There was a problem hiding this comment.
Same here, let's think about whether all of this should go into a single header. We can still have a convenience vortex.hpp header with only include statements.
2 participants
Replace C++ api with a new one which depends on vortex-ffi. Remove cxx from workspace. Port FFI examples and tests to C++ examples as tests.
Add vx_expression_clone to C FFI.
Remove const modifier from output from vx_expression_not