From 57abc2d413ce6cae303a6949a9b58cbd5a1a9f3d Mon Sep 17 00:00:00 2001 From: Johan Mabille Date: Tue, 16 Jul 2024 23:11:45 +0200 Subject: [PATCH] Replace reference proxy with nullable (#136) * Replaced reference proxies with nullable * Cleanup code * Workaround for clang * Workaround for clang everywhere ;) * Reenable tests for all the types * Changes according to review --- include/sparrow/array_data.hpp | 389 +----------------- include/sparrow/dictionary_encoded_layout.hpp | 220 +++++++--- include/sparrow/dynamic_bitset.hpp | 2 +- include/sparrow/fixed_size_layout.hpp | 10 +- include/sparrow/mp_utils.hpp | 2 +- include/sparrow/null_layout.hpp | 4 +- include/sparrow/nullable.hpp | 18 +- .../sparrow/variable_size_binary_layout.hpp | 8 +- test/CMakeLists.txt | 1 - test/test_array_data.cpp | 249 ----------- test/test_dictionary_encoded_layout.cpp | 23 +- test/test_fixed_size_layout.cpp | 4 +- test/test_null_layout.cpp | 12 +- test/test_typed_array.cpp | 2 +- test/test_typed_array_timestamp.cpp | 2 +- test/test_variable_size_binary_layout.cpp | 2 +- 16 files changed, 210 insertions(+), 738 deletions(-) delete mode 100644 test/test_array_data.cpp diff --git a/include/sparrow/array_data.hpp b/include/sparrow/array_data.hpp index e1d21723..12dfe2e3 100644 --- a/include/sparrow/array_data.hpp +++ b/include/sparrow/array_data.hpp @@ -53,153 +53,6 @@ namespace sparrow }; /** - * CRTP base class for reference types used in - * layout classes. The reference proxy classes - * inheriting from this class provide a similar - * behavior as std::optional, but as reference on - * data in an array_data. - */ - template - class reference_proxy_base - { - public: - - using self_type = reference_proxy_base; - using derived_type = D; - - derived_type& derived_cast(); - const derived_type& derived_cast() const; - - protected: - - reference_proxy_base() = default; - ~reference_proxy_base() = default; - - reference_proxy_base(const reference_proxy_base&) = default; - reference_proxy_base& operator=(const reference_proxy_base&) = default; - - reference_proxy_base(reference_proxy_base&&) = default; - reference_proxy_base& operator=(reference_proxy_base&&) = default; - }; - - template - concept ref_proxy = std::derived_from>; - - template - concept not_ref_proxy = (!ref_proxy); - - template - bool operator==(const reference_proxy_base& lhs, const reference_proxy_base& rhs); - - template - bool operator==(const reference_proxy_base& lhs, const T& rhs); - - template - bool operator==(const reference_proxy_base& lhs, std::nullopt_t); - - template - auto operator<=>(const reference_proxy_base& lhs, const reference_proxy_base& rhs); - - template - std::partial_ordering operator<=>(const reference_proxy_base& lhs, const T& rhs); - - template - std::strong_ordering operator<=>(const reference_proxy_base& lhs, std::nullopt_t); - - /** - * Default const reference proxy class - */ - template - class const_reference_proxy : public reference_proxy_base> - { - public: - - using self_type = const_reference_proxy; - using base_type = reference_proxy_base; - using layout_type = L; - using value_type = typename L::inner_value_type; - using const_reference = typename L::inner_const_reference; - using bitmap_const_reference = typename L::bitmap_const_reference; - using size_type = typename L::size_type; - - const_reference_proxy(const_reference val_ref, bitmap_const_reference bit_ref); - ~const_reference_proxy() = default; - - const_reference_proxy(const self_type&) = default; - const_reference_proxy(self_type&&) = default; - - bool has_value() const; - explicit operator bool() const; - - const_reference value() const; - - private: - - const_reference m_val_ref; - bitmap_const_reference m_bit_ref; - }; - - /** - * Default reference proxy class. - */ - template - class reference_proxy : public reference_proxy_base> - { - public: - - using self_type = reference_proxy; - using base_type = reference_proxy_base; - using layout_type = L; - using value_type = typename L::inner_value_type; - using reference = typename L::inner_reference; - using bitmap_reference = typename L::bitmap_reference; - using size_type = typename L::size_type; - - reference_proxy(reference val_ref, bitmap_reference bit_ref); - ~reference_proxy() = default; - - reference_proxy(const self_type&) = default; - reference_proxy(self_type&&) = default; - - bool has_value() const; - explicit operator bool() const; - - value_type& value(); - const value_type& value() const; - - self_type& operator=(const self_type& rhs); - self_type& operator=(self_type&& rhs); - self_type& operator=(std::nullopt_t); - - template U> - self_type& operator=(const std::optional& rhs); - - template U> - self_type& operator=(std::optional&& rhs); - - template U> - self_type& operator=(U&& value); - - void swap(self_type& rhs); - - private: - - void reset(); - - template - void update(U&& u); - - template - void update_value(U&& u); - - reference m_val_ref; - bitmap_reference m_bit_ref; - }; - - template - void swap(reference_proxy& lhs, reference_proxy& rhs); - - /* * Layout iterator class * * Relies on a layout's couple of value iterator and bitmap iterator to @@ -210,7 +63,7 @@ namespace sparrow layout_iterator, mpl::constify_t, typename L::iterator_tag, - std::conditional_t, reference_proxy>> + std::conditional_t> { public: @@ -219,7 +72,7 @@ namespace sparrow self_type, mpl::constify_t, typename L::iterator_tag, - std::conditional_t, reference_proxy>>; + std::conditional_t>; using reference = typename base_type::reference; using difference_type = typename base_type::difference_type; @@ -246,244 +99,6 @@ namespace sparrow friend class iterator_access; }; - /*************************************** - * reference_proxy_base implementation * - ***************************************/ - - template - auto reference_proxy_base::derived_cast() -> derived_type& - { - return *static_cast(this); - } - - template - auto reference_proxy_base::derived_cast() const -> const derived_type& - { - return *static_cast(this); - } - - template - bool operator==(const reference_proxy_base& lhs, const reference_proxy_base& rhs) - { - const D1& dlhs = lhs.derived_cast(); - const D2& drhs = rhs.derived_cast(); - return (dlhs && drhs && (dlhs.value() == drhs.value())) || (!dlhs && !drhs); - } - - template - bool operator==(const reference_proxy_base& lhs, const T& rhs) - { - return lhs.derived_cast() && (lhs.derived_cast().value() == rhs); - } - - template - bool operator==(const reference_proxy_base& lhs, std::nullopt_t) - { - return !lhs.derived_cast(); - } - - template - auto operator<=>(const reference_proxy_base& lhs, const reference_proxy_base& rhs) - { - const D1& dlhs = lhs.derived_cast(); - const D2& drhs = rhs.derived_cast(); - - using TOrdering = decltype(dlhs.value() <=> drhs.value()); - if (dlhs && drhs) - { - return dlhs.value() <=> drhs.value(); - } - return TOrdering(dlhs.has_value() <=> drhs.has_value()); - } - - template - std::partial_ordering operator<=>(const reference_proxy_base& lhs, const T& rhs) - { - if (lhs.derived_cast()) - { - return lhs.derived_cast().value() <=> rhs; - } - return std::partial_ordering::less; - } - - template - std::strong_ordering operator<=>(const reference_proxy_base& lhs, std::nullopt_t) - { - return lhs.derived_cast().has_value() <=> false; - } - - /**************************************** - * const_reference_proxy implementation * - ****************************************/ - - template - const_reference_proxy::const_reference_proxy(const_reference val_ref, bitmap_const_reference bit_ref) - : m_val_ref(val_ref) - , m_bit_ref(bit_ref) - { - } - - template - bool const_reference_proxy::has_value() const - { - return m_bit_ref; - } - - template - const_reference_proxy::operator bool() const - { - return has_value(); - } - - template - auto const_reference_proxy::value() const -> const_reference - { - SPARROW_ASSERT_TRUE(has_value()); - return m_val_ref; - } - - /********************************** - * reference_proxy implementation * - **********************************/ - - template - reference_proxy::reference_proxy(reference val_ref, bitmap_reference bit_ref) - : m_val_ref(val_ref) - , m_bit_ref(bit_ref) - { - } - - template - bool reference_proxy::has_value() const - { - return bool(m_bit_ref); - } - - template - reference_proxy::operator bool() const - { - return has_value(); - } - - template - auto reference_proxy::value() -> value_type& - { - SPARROW_ASSERT_TRUE(has_value()); - return m_val_ref; - } - - template - auto reference_proxy::value() const -> const value_type& - { - SPARROW_ASSERT_TRUE(has_value()); - return m_val_ref; - } - - template - auto reference_proxy::operator=(const self_type& rhs) -> self_type& - { - update(rhs); - return *this; - } - - template - auto reference_proxy::operator=(self_type&& rhs) -> self_type& - { - update(std::move(rhs)); - return *this; - } - - template - auto reference_proxy::operator=(std::nullopt_t) -> self_type& - { - reset(); - return *this; - } - - template - template U> - auto reference_proxy::operator=(const std::optional& rhs) -> self_type& - { - update(rhs); - return *this; - } - - template - template U> - auto reference_proxy::operator=(std::optional&& rhs) -> self_type& - { - update(std::move(rhs)); - return *this; - } - - template - template U> - auto reference_proxy::operator=(U&& value) -> self_type& - { - update_value(std::forward(value)); - return *this; - } - - template - void reference_proxy::reset() - { - m_bit_ref = false; - } - - template - void reference_proxy::swap(self_type& rhs) - { - const bool this_has_value = has_value(); - const bool rhs_has_value = rhs.has_value(); - using std::swap; - if (this_has_value) - { - if (rhs_has_value) - { - swap(m_val_ref, rhs.m_val_ref); - } - else - { - rhs.update_value(value()); - reset(); - } - } - else if (rhs_has_value) - { - update_value(rhs.value()); - rhs.reset(); - } - // If none has value, nothing to do - } - - template - template - void reference_proxy::update(U&& u) - { - if (u.has_value()) - { - update_value(std::forward(u).value()); - } - else - { - reset(); - } - } - - template - template - void reference_proxy::update_value(U&& u) - { - m_bit_ref = true; - m_val_ref = std::forward(u); - } - - template - void swap(reference_proxy& lhs, reference_proxy& rhs) - { - lhs.swap(rhs); - } - /********************************** * layout_iterator implementation * **********************************/ diff --git a/include/sparrow/dictionary_encoded_layout.hpp b/include/sparrow/dictionary_encoded_layout.hpp index e4bfb3f3..cfe00a57 100644 --- a/include/sparrow/dictionary_encoded_layout.hpp +++ b/include/sparrow/dictionary_encoded_layout.hpp @@ -22,41 +22,100 @@ namespace sparrow { + /* + * Traits class for the iterator over the data values + * of a dictionary encoded layout. + * + * @tparam L the layout type + * @tparam IC a constnat indicating whether the inner types + * must be defined for a constant iterator. + */ + template + struct dictionary_value_traits + { + using layout_type = L; + using value_type = typename L::inner_value_type; + using tag = std::random_access_iterator_tag; + using const_reference = typename L::inner_const_reference; + static constexpr bool is_value = true; + static constexpr bool is_const = IC; + }; + + /* + * Traits class for the iterator over the bitmap values + * of a dictionary encoded layout. + * + * @tparam L the layout type + * @tparam IC a constnat indicating whether the inner types + * must be defined for a constant iterator. + */ + template + struct dictionary_bitmap_traits + { + using layout_type = L; + using value_type = typename L::bitmap_value_type; + using tag = std::random_access_iterator_tag; + using const_reference = typename L::bitmap_const_reference; + static constexpr bool is_value = false; + static constexpr bool is_const = IC; + }; + + template + concept dictionary_iterator_traits = requires + { + typename T::layout_type; + typename T::value_type; + typename T::tag; + typename T::const_reference; + { T::is_value } -> std::same_as; + { T::is_const } -> std::same_as; + }; + /** - * @class dictionary_value_iterator + * @class dictionary_iterator * - * @brief Iterator over the data values of a dictionary layout. + * @brief Iterator over the values or the bitmap elements of a dictionary layout. * - * @tparam IL the layout type of the indexes. - * @tparam SL the layout type of the dictionary. - * @tparam is_const a boolean flag specifying whether this iterator is const. + * @tparam Traits the traits defining the inner types of the iterator. */ - template - class dictionary_value_iterator : public iterator_base< - dictionary_value_iterator, - typename SL::value_type, - std::random_access_iterator_tag, - typename SL::const_reference> + template + class dictionary_iterator : public iterator_base< + dictionary_iterator, + typename Traits::value_type, + typename Traits::tag, + typename Traits::const_reference> { public: - using self_type = dictionary_value_iterator; - using base_type = iterator_base; + using self_type = dictionary_iterator; + using base_type = iterator_base< + dictionary_iterator, + typename Traits::value_type, + typename Traits::tag, + typename Traits::const_reference>; using reference = typename base_type::reference; using difference_type = typename base_type::difference_type; - using index_iterator = std::conditional_t; - using sub_layout = mpl::constify_t; - using sub_layout_reference = sub_layout&; + using layout_type = typename Traits::layout_type; + using index_layout = typename layout_type::indexes_layout; + using index_iterator = std::conditional_t; + using sub_layout = typename layout_type::sub_layout; + using sub_layout_storage = mpl::constify_t; + using sub_layout_reference = sub_layout_storage&; - // `dictionary_value_iterator` needs to be default constructible + // `dictionary_iterator` needs to be default constructible // to satisfy `dictionary_encoded_layout::const_value_range`'s + // and `dicitonary_encoded_layout::const_bitmap_range`'s // constraints. - dictionary_value_iterator() noexcept = default; - dictionary_value_iterator(index_iterator index_it, sub_layout_reference sub_layout_reference); + dictionary_iterator() noexcept = default; + dictionary_iterator(index_iterator index_it, sub_layout_reference sub_layout_reference); private: + using sub_reference = std::conditional_t; + + sub_reference get_subreference() const; + reference dereference() const; void increment(); void decrement(); @@ -67,7 +126,7 @@ namespace sparrow index_iterator m_index_it; // Use std::optional because of default constructor. - std::optional> m_sub_layout_reference; + std::optional> m_sub_layout_reference; friend class iterator_access; }; @@ -105,15 +164,16 @@ namespace sparrow using self_type = dictionary_encoded_layout; using index_type = IT; - using inner_value_type = SL::inner_value_type; using sub_layout = SL; - using inner_reference = reference_proxy; - using inner_const_reference = const_reference_proxy; + using inner_value_type = SL::inner_value_type; + using inner_reference = typename SL::inner_reference; + using inner_const_reference = typename SL::inner_const_reference; using bitmap_type = array_data::bitmap_type; + using bitmap_value_type = bitmap_type::value_type; using bitmap_const_reference = bitmap_type::const_reference; using value_type = SL::value_type; - using reference = reference_proxy; - using const_reference = const_reference_proxy; + using reference = typename SL::reference; + using const_reference = typename SL::const_reference; using size_type = std::size_t; using indexes_layout = fixed_size_layout; using iterator_tag = std::random_access_iterator_tag; @@ -134,13 +194,13 @@ namespace sparrow using iterator = layout_iterator; using const_iterator = layout_iterator; - using bitmap_iterator = indexes_layout::bitmap_iterator; - using const_bitmap_iterator = indexes_layout::const_bitmap_iterator; - using const_bitmap_range = indexes_layout::const_bitmap_range; + using bitmap_iterator = dictionary_iterator>; + using const_bitmap_iterator = dictionary_iterator>; + using const_bitmap_range = std::ranges::subrange; - using value_iterator = dictionary_value_iterator; - using const_value_iterator = dictionary_value_iterator; - using const_value_range = std::ranges::subrange; + using value_iterator = dictionary_iterator>; + using const_value_iterator = dictionary_iterator>; + using const_value_range = std::ranges::subrange; explicit dictionary_encoded_layout(array_data& data); void rebind_data(array_data& data); @@ -166,6 +226,9 @@ namespace sparrow const_value_iterator value_cbegin() const; const_value_iterator value_cend() const; + const_bitmap_iterator bitmap_cbegin() const; + const_bitmap_iterator bitmap_cend() const; + inner_const_reference value(size_type i) const; const_offset_iterator offset(size_type i) const; @@ -183,16 +246,15 @@ namespace sparrow return instance; } - friend class const_reference_proxy; - friend class dictionary_value_iterator; + friend class dictionary_iterator>; }; /******************************************* * vs_binary_value_iterator implementation * *******************************************/ - template - dictionary_value_iterator::dictionary_value_iterator( + template + dictionary_iterator::dictionary_iterator( index_iterator index_it, sub_layout_reference sub_layout_ref ) @@ -201,45 +263,65 @@ namespace sparrow { } - template - auto dictionary_value_iterator::dereference() const -> reference + template + auto dictionary_iterator::get_subreference() const -> sub_reference + { + return (*m_sub_layout_reference).get()[m_index_it->value()]; + } + + template + auto dictionary_iterator::dereference() const -> reference { SPARROW_ASSERT_TRUE(m_sub_layout_reference.has_value()); - return (*m_sub_layout_reference).get()[*m_index_it]; + if constexpr (Traits::is_value) + { + if (m_index_it->has_value()) + { + return get_subreference().get(); + } + else + { + return layout_type::dummy_const_reference().get(); + } + } + else + { + return m_index_it->has_value() && get_subreference().has_value(); + } } - template - void dictionary_value_iterator::increment() + template + void dictionary_iterator::increment() { ++m_index_it; } - template - void dictionary_value_iterator::decrement() + template + void dictionary_iterator::decrement() { --m_index_it; } - template - void dictionary_value_iterator::advance(difference_type n) + template + void dictionary_iterator::advance(difference_type n) { m_index_it += n; } - template - auto dictionary_value_iterator::distance_to(const self_type& rhs) const -> difference_type + template + auto dictionary_iterator::distance_to(const self_type& rhs) const -> difference_type { m_index_it.distance_to(rhs.m_index_it); } - template - bool dictionary_value_iterator::equal(const self_type& rhs) const + template + bool dictionary_iterator::equal(const self_type& rhs) const { return m_index_it == rhs.m_index_it; } - template - bool dictionary_value_iterator::less_than(const self_type& rhs) const + template + bool dictionary_iterator::less_than(const self_type& rhs) const { return m_index_it < rhs.m_index_it; } @@ -285,46 +367,58 @@ namespace sparrow } template - auto dictionary_encoded_layout::bitmap() const -> const_bitmap_range + auto dictionary_encoded_layout::cbegin() const -> const_iterator { - return get_const_indexes_layout().bitmap(); + return const_iterator(value_cbegin(), bitmap_cbegin()); } template - const typename dictionary_encoded_layout::indexes_layout& - dictionary_encoded_layout::get_const_indexes_layout() const + auto dictionary_encoded_layout::cend() const -> const_iterator { - return *const_cast(m_indexes_layout.get()); + return const_iterator(value_cend(), bitmap_cend()); } + template - auto dictionary_encoded_layout::cbegin() const -> const_iterator + auto dictionary_encoded_layout::bitmap() const -> const_bitmap_range { - return const_iterator(value_cbegin(), get_const_indexes_layout().bitmap().begin()); + return const_bitmap_range(bitmap_cbegin(), bitmap_cend()); } template - auto dictionary_encoded_layout::cend() const -> const_iterator + auto dictionary_encoded_layout::values() const -> const_value_range { - return const_iterator(value_cend(), get_const_indexes_layout().bitmap().end()); + return const_value_range(value_cbegin(), value_cend()); + } + + template + const typename dictionary_encoded_layout::indexes_layout& + dictionary_encoded_layout::get_const_indexes_layout() const + { + return *const_cast(m_indexes_layout.get()); } template auto dictionary_encoded_layout::value_cbegin() const -> const_value_iterator { - return const_value_iterator(get_const_indexes_layout().values().begin(), *m_sub_layout); + return const_value_iterator(get_const_indexes_layout().cbegin(), *m_sub_layout); } template auto dictionary_encoded_layout::value_cend() const -> const_value_iterator { - return const_value_iterator(get_const_indexes_layout().values().end(), *m_sub_layout); + return const_value_iterator(get_const_indexes_layout().cend(), *m_sub_layout); + } + template + auto dictionary_encoded_layout::bitmap_cbegin() const -> const_bitmap_iterator + { + return const_bitmap_iterator(get_const_indexes_layout().cbegin(), *m_sub_layout); } template - auto dictionary_encoded_layout::values() const -> const_value_range + auto dictionary_encoded_layout::bitmap_cend() const -> const_bitmap_iterator { - return const_value_range(value_cbegin(), value_cend()); + return const_bitmap_iterator(get_const_indexes_layout().cend(), *m_sub_layout); } template diff --git a/include/sparrow/dynamic_bitset.hpp b/include/sparrow/dynamic_bitset.hpp index 8281500a..edc7231b 100644 --- a/include/sparrow/dynamic_bitset.hpp +++ b/include/sparrow/dynamic_bitset.hpp @@ -205,7 +205,7 @@ namespace sparrow self_type& operator=(self_type&&) noexcept; self_type& operator=(bool) noexcept; - explicit operator bool() const noexcept; + operator bool() const noexcept; bool operator~() const noexcept; diff --git a/include/sparrow/fixed_size_layout.hpp b/include/sparrow/fixed_size_layout.hpp index 740a014e..8c7bdd46 100644 --- a/include/sparrow/fixed_size_layout.hpp +++ b/include/sparrow/fixed_size_layout.hpp @@ -25,6 +25,7 @@ #include "sparrow/contracts.hpp" #include "sparrow/dynamic_bitset.hpp" #include "sparrow/iterator.hpp" +#include "sparrow/nullable.hpp" namespace sparrow { @@ -51,9 +52,9 @@ namespace sparrow using bitmap_type = array_data::bitmap_type; using bitmap_reference = typename bitmap_type::reference; using bitmap_const_reference = typename bitmap_type::const_reference; - using value_type = std::optional; - using reference = reference_proxy; - using const_reference = const_reference_proxy; + using value_type = nullable; + using reference = nullable; + using const_reference = nullable; using pointer = inner_value_type*; using const_pointer = const inner_value_type*; using size_type = std::size_t; @@ -120,9 +121,6 @@ namespace sparrow const array_data& data_ref() const; std::reference_wrapper m_data; - - friend class reference_proxy; - friend class const_reference_proxy; }; /************************************ diff --git a/include/sparrow/mp_utils.hpp b/include/sparrow/mp_utils.hpp index 61114016..cfdd8737 100644 --- a/include/sparrow/mp_utils.hpp +++ b/include/sparrow/mp_utils.hpp @@ -422,6 +422,6 @@ namespace sparrow::mpl /// Matches types that can be convertible to and assignable from bool. We do not use /// `std::convertible_to` because we don't want to impose an implicit conversion. template - concept boolean_like = std::is_assignable_v, bool> and + concept boolean_like = std::is_assignable_v>, bool> and requires { static_cast(std::declval()); }; } diff --git a/include/sparrow/null_layout.hpp b/include/sparrow/null_layout.hpp index a553402b..07111ef9 100644 --- a/include/sparrow/null_layout.hpp +++ b/include/sparrow/null_layout.hpp @@ -16,13 +16,13 @@ #include #include -#include #include #include "sparrow/array_data.hpp" #include "sparrow/contracts.hpp" #include "sparrow/data_type.hpp" #include "sparrow/iterator.hpp" +#include "sparrow/nullable.hpp" namespace sparrow { @@ -74,7 +74,7 @@ namespace sparrow public: using inner_value_type = null_type; - using value_type = std::optional; + using value_type = nullable; using iterator = empty_iterator; using const_iterator = empty_iterator; using reference = iterator::reference; diff --git a/include/sparrow/nullable.hpp b/include/sparrow/nullable.hpp index 86cc571b..7bfaa082 100644 --- a/include/sparrow/nullable.hpp +++ b/include/sparrow/nullable.hpp @@ -21,6 +21,20 @@ #include "sparrow/mp_utils.hpp" +#if defined(SPARROW_CONSTEXPR) +#error "SPARROW_CONSTEXPR already defined" +#endif + +// clang workaround: clang instantiates the constructor in SFINAE context, +// which is incompatible with the implementation of standard libraries which +// are not libc++.This leads to wrong compilation errors. Making the constructor +// not constexpr prevents the compiler from instantiating it. +#if defined(__clang__) && not defined(_LIBCPP_VERSION) +# define SPARROW_CONSTEXPR +#else +# define SPARROW_CONSTEXPR constexpr +#endif + namespace sparrow { template @@ -294,7 +308,7 @@ namespace sparrow not impl::initializable_from_refs> ) explicit(not impl::both_convertible_from_cond_ref) - constexpr nullable(nullable&& rhs) + SPARROW_CONSTEXPR nullable(nullable&& rhs) : m_value(std::move(rhs).get()) , m_null_flag(std::move(rhs).null_flag()) { @@ -658,3 +672,5 @@ namespace sparrow } } +#undef SPARROW_CONSTEXPR + diff --git a/include/sparrow/variable_size_binary_layout.hpp b/include/sparrow/variable_size_binary_layout.hpp index dac5da4b..2dc7c7a7 100644 --- a/include/sparrow/variable_size_binary_layout.hpp +++ b/include/sparrow/variable_size_binary_layout.hpp @@ -21,6 +21,7 @@ #include "sparrow/contracts.hpp" #include "sparrow/iterator.hpp" #include "sparrow/mp_utils.hpp" +#include "sparrow/nullable.hpp" namespace sparrow { @@ -109,9 +110,9 @@ namespace sparrow using inner_const_reference = CR; using bitmap_type = array_data::bitmap_type; using bitmap_const_reference = typename bitmap_type::const_reference; - using value_type = std::optional; - using reference = const_reference_proxy; - using const_reference = const_reference_proxy; + using value_type = nullable; + using reference = nullable; + using const_reference = nullable; using size_type = std::size_t; using iterator_tag = std::contiguous_iterator_tag; @@ -180,7 +181,6 @@ namespace sparrow std::reference_wrapper m_data; - friend class const_reference_proxy; friend class vs_binary_value_iterator; }; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 0e30169f..a84a3166 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -38,7 +38,6 @@ set(SPARROW_TESTS_SOURCES test_algorithm.cpp test_allocator.cpp test_array.cpp - test_array_data.cpp test_array_data_concepts.cpp test_array_data_creation.cpp test_array_data_factory.cpp diff --git a/test/test_array_data.cpp b/test/test_array_data.cpp deleted file mode 100644 index 89491eb4..00000000 --- a/test/test_array_data.cpp +++ /dev/null @@ -1,249 +0,0 @@ -// Copyright 2024 Man Group Operations Limited -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#include - -#include "sparrow/array_data.hpp" -#include "sparrow/contracts.hpp" - -#include "doctest/doctest.h" - -namespace sparrow -{ - class mock_layout - { - public: - - using value_type = std::optional; - using inner_value_type = value_type::value_type; - using inner_reference = int&; - using inner_const_reference = const int&; - using bitmap_type = array_data::bitmap_type; - using bitmap_reference = typename bitmap_type::reference; - using bitmap_const_reference = typename bitmap_type::const_reference; - using reference = reference_proxy; - using const_reference = const_reference_proxy; - using size_type = std::size_t; - - mock_layout() = default; - - mock_layout(std::initializer_list l) - : m_bitmap(l.size()) - , m_data(l.size()) - { - auto bit_iter = m_bitmap.begin(); - auto value_iter = m_data.begin(); - for (const auto& v : l) - { - *bit_iter++ = v.has_value(); - if (v.has_value()) - { - *value_iter++ = v.value(); - } - else - { - ++value_iter; - } - } - } - - reference operator[](size_type pos) - { - SPARROW_ASSERT_TRUE(pos < m_bitmap.size()); - SPARROW_ASSERT_TRUE(pos < m_data.size()); - return reference(m_data[pos], m_bitmap[pos]); - } - - const_reference operator[](size_type pos) const - { - SPARROW_ASSERT_TRUE(pos < m_bitmap.size()); - SPARROW_ASSERT_TRUE(pos < m_data.size()); - return const_reference(m_data[pos], m_bitmap[pos]); - } - - bool has_value(size_type pos) const - { - SPARROW_ASSERT_TRUE(pos < m_bitmap.size()); - return m_bitmap[pos]; - } - - inner_const_reference value(size_type pos) const - { - SPARROW_ASSERT_TRUE(pos < m_data.size()); - return m_data[pos]; - } - - private: - - bitmap_type m_bitmap; - std::vector m_data; - }; - - struct ref_proxy_fixture - { - ref_proxy_fixture() - { - m_layout = {std::make_optional(2), std::make_optional(5), std::nullopt, std::make_optional(7)}; - } - - const mock_layout& layout() const - { - return m_layout; - } - - mock_layout m_layout; - }; - - TEST_SUITE("reference_proxy") - { - TEST_CASE_FIXTURE(ref_proxy_fixture, "has_value") - { - auto ref0 = m_layout[0]; - CHECK(ref0.has_value()); - CHECK(ref0); - - auto ref2 = m_layout[2]; - CHECK(!ref2.has_value()); - CHECK(!ref2); - } - - TEST_CASE_FIXTURE(ref_proxy_fixture, "value") - { - auto ref0 = m_layout[0]; - CHECK_EQ(ref0.value(), m_layout.value(0)); - static_assert(std::same_as); - - const auto cref0 = m_layout[0]; - CHECK_EQ(cref0.value(), m_layout.value(0)); - static_assert(std::same_as); - } - - TEST_CASE_FIXTURE(ref_proxy_fixture, "assign") - { - int expected_value0 = 4; - auto ref0 = m_layout[0]; - ref0 = expected_value0; - CHECK_EQ(m_layout.value(0), expected_value0); - - ref0 = std::nullopt; - CHECK(!ref0.has_value()); - - ref0 = expected_value0; - CHECK_EQ(m_layout.value(0), expected_value0); - - int expected_value2 = 3; - auto ref2 = m_layout[2]; - ref2 = std::make_optional(expected_value2); - CHECK_EQ(m_layout.value(2), expected_value2); - - ref0 = ref2; - CHECK_EQ(m_layout.value(0), expected_value2); - - int expected_value3 = 8; - std::optional opt(expected_value3); - auto ref3 = m_layout[3]; - ref3 = opt; - CHECK_EQ(m_layout.value(3), expected_value3); - - ref0 = std::move(ref3); - CHECK_EQ(m_layout.value(0), expected_value3); - } - - TEST_CASE_FIXTURE(ref_proxy_fixture, "swap") - { - int expected_value0 = m_layout.value(0); - int expected_value3 = m_layout.value(3); - - auto ref0 = m_layout[0]; - auto ref2 = m_layout[2]; - auto ref3 = m_layout[3]; - - ref0.swap(ref2); - CHECK(!ref0); - CHECK(ref2); - CHECK_EQ(ref2.value(), expected_value0); - - swap(ref2, ref3); - CHECK_EQ(ref2.value(), expected_value3); - CHECK_EQ(ref3.value(), expected_value0); - } - - TEST_CASE_FIXTURE(ref_proxy_fixture, "comparison") - { - SUBCASE("equality") - { - auto ref0 = m_layout[0]; - auto ref1 = m_layout[1]; - auto ref2 = m_layout[2]; - - CHECK(ref0 != ref1); - CHECK(ref0 != m_layout.value(1)); - CHECK(m_layout.value(0) != ref1); - ref0 = ref1; - CHECK(ref0 == ref1); - CHECK(ref0 == m_layout.value(1)); - CHECK(m_layout.value(0) == ref1); - - CHECK(ref0 != ref2); - } - - SUBCASE("inequality") - { - auto ref0 = m_layout[0]; - auto ref2 = m_layout[2]; - auto ref3 = m_layout[3]; - - CHECK(ref2 < ref0); - CHECK(ref2 <= ref0); - CHECK(ref3 > ref2); - CHECK(ref3 >= ref2); - - CHECK(ref0 < ref3); - CHECK(ref0 <= ref3); - CHECK(ref0.value() < ref3); - CHECK(ref0.value() <= ref3); - CHECK(ref0 < ref3.value()); - CHECK(ref0 <= ref3.value()); - - CHECK(ref3 > ref0); - CHECK(ref3 >= ref0); - CHECK(ref3.value() > ref0); - CHECK(ref3.value() >= ref0); - CHECK(ref3 > ref0.value()); - CHECK(ref3 >= ref0.value()); - } - } - } - - TEST_SUITE("const_reference_proxy") - { - TEST_CASE_FIXTURE(ref_proxy_fixture, "has_value") - { - auto ref0 = layout()[0]; - CHECK(ref0.has_value()); - CHECK(ref0); - - auto ref2 = layout()[2]; - CHECK(!ref2.has_value()); - CHECK(!ref2); - } - - TEST_CASE_FIXTURE(ref_proxy_fixture, "value") - { - auto ref0 = layout()[0]; - CHECK_EQ(ref0.value(), m_layout.value(0)); - static_assert(std::same_as); - } - } -} diff --git a/test/test_dictionary_encoded_layout.cpp b/test/test_dictionary_encoded_layout.cpp index 12e5c450..c96d0b0c 100644 --- a/test/test_dictionary_encoded_layout.cpp +++ b/test/test_dictionary_encoded_layout.cpp @@ -169,8 +169,7 @@ namespace sparrow CHECK(iter->has_value()); CHECK_EQ(iter->value(), l[7]); ++iter; - CHECK(iter->has_value()); - CHECK_EQ(iter->value(), l[8]); + CHECK_FALSE(iter->has_value()); ++iter; CHECK_FALSE(iter->has_value()); ++iter; @@ -182,26 +181,26 @@ namespace sparrow layout_type l(m_data); const auto vrange = l.values(); auto iter = vrange.begin(); - CHECK_EQ(iter->value(), l[0].value()); + CHECK_EQ(*iter, l[0].value()); ++iter; --iter; - CHECK_EQ(iter->value(), l[0].value()); + CHECK_EQ(*iter, l[0].value()); iter += 2; - CHECK_EQ(iter->value(), l[2].value()); + CHECK_EQ(*iter, l[2].value()); ++iter; - CHECK_EQ(iter->value(), l[3].value()); + CHECK_EQ(*iter, l[3].value()); ++iter; - CHECK_EQ(iter->value(), l[4].value()); + CHECK_EQ(*iter, l[4].value()); ++iter; - CHECK_EQ(iter->value(), l[5].value()); + CHECK_EQ(*iter, l[5].value()); ++iter; - CHECK_EQ(iter->value(), l[6].value()); + CHECK_EQ(*iter, l[6].value()); ++iter; - CHECK_EQ(iter->value(), l[7].value()); + CHECK_EQ(*iter, l[7].value()); ++iter; - CHECK_EQ(iter->has_value(), l[8].has_value()); + CHECK_EQ(*iter, l[8].get()); ++iter; - CHECK_EQ(iter->value(), words[2]); + CHECK_NE(*iter, words[2]); ++iter; CHECK_EQ(iter, vrange.end()); } diff --git a/test/test_fixed_size_layout.cpp b/test/test_fixed_size_layout.cpp index 9e6e4373..f7cdf29b 100644 --- a/test/test_fixed_size_layout.cpp +++ b/test/test_fixed_size_layout.cpp @@ -135,7 +135,7 @@ namespace sparrow { if (i % 2 != 0) { - lt[i] = std::nullopt; + lt[i] = nullval; } } @@ -155,7 +155,7 @@ namespace sparrow for (std::size_t i = 0; i != lt.size(); ++it, ++i) { - CHECK_EQ(*it, std::make_optional(lt[i].value())); + CHECK_EQ(*it, make_nullable(lt[i].value())); CHECK(it->has_value()); } diff --git a/test/test_null_layout.cpp b/test/test_null_layout.cpp index 26a6a069..3a666dc6 100644 --- a/test/test_null_layout.cpp +++ b/test/test_null_layout.cpp @@ -48,8 +48,8 @@ namespace sparrow null_layout nl(ad); const null_layout cnl(ad); - CHECK_EQ(nl[2], std::nullopt); - CHECK_EQ(cnl[2], std::nullopt); + CHECK_EQ(nl[2], nullval); + CHECK_EQ(cnl[2], nullval); } TEST_CASE("iterator") @@ -59,13 +59,13 @@ namespace sparrow auto iter = nl.begin(); auto citer = nl.cbegin(); - CHECK_EQ(*iter, std::nullopt); - CHECK_EQ(*citer, std::nullopt); + CHECK_EQ(*iter, nullval); + CHECK_EQ(*citer, nullval); ++iter; ++citer; - CHECK_EQ(*iter, std::nullopt); - CHECK_EQ(*citer, std::nullopt); + CHECK_EQ(*iter, nullval); + CHECK_EQ(*citer, nullval); iter += 2; citer += 2; diff --git a/test/test_typed_array.cpp b/test/test_typed_array.cpp index 88d5ecaf..3292bc8a 100644 --- a/test/test_typed_array.cpp +++ b/test/test_typed_array.cpp @@ -232,7 +232,7 @@ TEST_SUITE("typed_array") for (typename typed_array::size_type i = 0; i < ta.size() - 1; ++iter, ++i) { REQUIRE(iter->has_value()); - CHECK_EQ(*iter, std::make_optional(ta[i].value())); + CHECK_EQ(*iter, make_nullable(ta[i].value())); } CHECK_EQ(++iter, end); diff --git a/test/test_typed_array_timestamp.cpp b/test/test_typed_array_timestamp.cpp index cfe5ef5d..f5acf927 100644 --- a/test/test_typed_array_timestamp.cpp +++ b/test/test_typed_array_timestamp.cpp @@ -181,7 +181,7 @@ TEST_SUITE("typed_array_timestamp") for (typename sparrow::typed_array::size_type i = 0; i < ta.size() - 1; ++iter, ++i) { REQUIRE(iter->has_value()); - CHECK_EQ(*iter, std::make_optional(ta[i].value())); + CHECK_EQ(*iter, sparrow::make_nullable(ta[i].value())); } CHECK_EQ(++iter, end); diff --git a/test/test_variable_size_binary_layout.cpp b/test/test_variable_size_binary_layout.cpp index 892c4ff9..d4c479de 100644 --- a/test/test_variable_size_binary_layout.cpp +++ b/test/test_variable_size_binary_layout.cpp @@ -126,7 +126,7 @@ namespace sparrow auto cref2 = l[2]; auto iter = l.cbegin(); - CHECK_EQ(*iter, std::make_optional(cref0.value())); + CHECK_EQ(*iter, make_nullable(cref0.value())); ++iter; CHECK(!iter->has_value());