From 5ab696941f0d5f9c58797b83ff833e9987ce404e Mon Sep 17 00:00:00 2001 From: Anton Alkin Date: Tue, 5 Mar 2024 11:00:04 +0100 Subject: [PATCH 1/3] DPL Analysis: update slice index * If the source object is Filtered, the returned slice is also Filtered * The raw iterator preserves correct paren_t if returned from a Filtered object * Recursive grouped self-index test added --- Framework/Core/include/Framework/ASoA.h | 278 ++++++++++++------ .../Core/include/Framework/AnalysisTask.h | 12 +- Framework/Core/test/test_GroupSlicer.cxx | 129 +++++++- 3 files changed, 318 insertions(+), 101 deletions(-) diff --git a/Framework/Core/include/Framework/ASoA.h b/Framework/Core/include/Framework/ASoA.h index e83cf8d0ca326..4035d94fd433d 100644 --- a/Framework/Core/include/Framework/ASoA.h +++ b/Framework/Core/include/Framework/ASoA.h @@ -585,95 +585,17 @@ struct RowViewSentinel { uint64_t const index; }; -struct DefaultIndexPolicy : IndexPolicyBase { - /// Needed to be able to copy the policy - DefaultIndexPolicy() = default; - DefaultIndexPolicy(DefaultIndexPolicy&&) = default; - DefaultIndexPolicy(DefaultIndexPolicy const&) = default; - DefaultIndexPolicy& operator=(DefaultIndexPolicy const&) = default; - DefaultIndexPolicy& operator=(DefaultIndexPolicy&&) = default; - - /// mMaxRow is one behind the last row, so effectively equal to the number of - /// rows @a nRows. Offset indicates that the index is actually part of - /// a larger - DefaultIndexPolicy(int64_t nRows, uint64_t offset) - : IndexPolicyBase{0, offset}, - mMaxRow(nRows) - { - } - - void limitRange(int64_t start, int64_t end) - { - this->setCursor(start); - if (end >= 0) { - mMaxRow = std::min(end, mMaxRow); - } - } - - [[nodiscard]] std::tuple - getIndices() const - { - return std::make_tuple(&mRowIndex, &mRowIndex); - } - - [[nodiscard]] std::tuple - getOffsets() const - { - return std::make_tuple(&mOffset); - } - - void setCursor(int64_t i) - { - this->mRowIndex = i; - } - void moveByIndex(int64_t i) - { - this->mRowIndex += i; - } - - void moveToEnd() - { - this->setCursor(mMaxRow); - } - - friend bool operator!=(DefaultIndexPolicy const& lh, DefaultIndexPolicy const& rh) - { - return O2_BUILTIN_LIKELY(lh.mRowIndex != rh.mRowIndex); - } - - friend bool operator==(DefaultIndexPolicy const& lh, DefaultIndexPolicy const& rh) - { - return O2_BUILTIN_UNLIKELY(lh.mRowIndex == rh.mRowIndex); - } - - bool operator!=(RowViewSentinel const& sentinel) const - { - return O2_BUILTIN_LIKELY(this->mRowIndex != sentinel.index); - } - - bool operator==(RowViewSentinel const& sentinel) const - { - return O2_BUILTIN_UNLIKELY(this->mRowIndex == sentinel.index); - } - - [[nodiscard]] auto size() const - { - return mMaxRow; - } - - int64_t mMaxRow = 0; -}; - struct FilteredIndexPolicy : IndexPolicyBase { // We use -1 in the IndexPolicyBase to indicate that the index is // invalid. What will validate the index is the this->setCursor() // which happens below which will properly setup the first index // by remapping the filtered index 0 to whatever unfiltered index // it belongs to. - FilteredIndexPolicy(gsl::span selection, uint64_t offset = 0) + FilteredIndexPolicy(gsl::span selection, int64_t rows, uint64_t offset = 0) : IndexPolicyBase{-1, offset}, mSelectedRows(selection), - mMaxSelection(selection.size()) + mMaxSelection(selection.size()), + nRows{rows} { this->setCursor(0); } @@ -743,9 +665,9 @@ struct FilteredIndexPolicy : IndexPolicyBase { return O2_BUILTIN_UNLIKELY(mSelectionRow == sentinel.index); } - /// Move iterator to one after the end. Since this is a view - /// we move the mSelectionRow to one past the view size and - /// the mRowIndex to one past the last entry in the selection + /// Move iterator to one after the end. Since this is a view + /// we move the mSelectionRow to one past the view size and + /// the mRowIndex to one past the last entry in the selection void moveToEnd() { this->mSelectionRow = this->mMaxSelection; @@ -762,6 +684,11 @@ struct FilteredIndexPolicy : IndexPolicyBase { return mMaxSelection; } + [[nodiscard]] auto raw_size() const + { + return nRows; + } + private: inline void updateRow() { @@ -770,6 +697,92 @@ struct FilteredIndexPolicy : IndexPolicyBase { gsl::span mSelectedRows; int64_t mSelectionRow = 0; int64_t mMaxSelection = 0; + int64_t nRows = 0; +}; + +struct DefaultIndexPolicy : IndexPolicyBase { + /// Needed to be able to copy the policy + DefaultIndexPolicy() = default; + DefaultIndexPolicy(DefaultIndexPolicy&&) = default; + DefaultIndexPolicy(DefaultIndexPolicy const&) = default; + DefaultIndexPolicy& operator=(DefaultIndexPolicy const&) = default; + DefaultIndexPolicy& operator=(DefaultIndexPolicy&&) = default; + + /// mMaxRow is one behind the last row, so effectively equal to the number of + /// rows @a nRows. Offset indicates that the index is actually part of + /// a larger + DefaultIndexPolicy(int64_t nRows, uint64_t offset) + : IndexPolicyBase{0, offset}, + mMaxRow(nRows) + { + } + + DefaultIndexPolicy(FilteredIndexPolicy const& other) + : IndexPolicyBase{0, other.mOffset}, + mMaxRow(other.raw_size()) + { + } + + void limitRange(int64_t start, int64_t end) + { + this->setCursor(start); + if (end >= 0) { + mMaxRow = std::min(end, mMaxRow); + } + } + + [[nodiscard]] std::tuple + getIndices() const + { + return std::make_tuple(&mRowIndex, &mRowIndex); + } + + [[nodiscard]] std::tuple + getOffsets() const + { + return std::make_tuple(&mOffset); + } + + void setCursor(int64_t i) + { + this->mRowIndex = i; + } + void moveByIndex(int64_t i) + { + this->mRowIndex += i; + } + + void moveToEnd() + { + this->setCursor(mMaxRow); + } + + friend bool operator!=(DefaultIndexPolicy const& lh, DefaultIndexPolicy const& rh) + { + return O2_BUILTIN_LIKELY(lh.mRowIndex != rh.mRowIndex); + } + + friend bool operator==(DefaultIndexPolicy const& lh, DefaultIndexPolicy const& rh) + { + return O2_BUILTIN_UNLIKELY(lh.mRowIndex == rh.mRowIndex); + } + + bool operator!=(RowViewSentinel const& sentinel) const + { + return O2_BUILTIN_LIKELY(this->mRowIndex != sentinel.index); + } + + bool operator==(RowViewSentinel const& sentinel) const + { + return O2_BUILTIN_UNLIKELY(this->mRowIndex == sentinel.index); + } + + [[nodiscard]] auto size() const + { + return mMaxRow; + } + + int64_t mMaxRow = 0; }; template @@ -829,6 +842,14 @@ struct RowViewCore : public IP, C... { return *this; } + RowViewCore(RowViewCore const& other) requires std::is_same_v + : IP{static_cast(other)}, + C(static_cast(other))... + { + bindIterators(persistent_columns_t{}); + bindAllDynamicColumns(dynamic_columns_t{}); + } + RowViewCore& operator++() { this->moveByIndex(1); @@ -1403,6 +1424,13 @@ class Table return *this; } + template + RowViewBase& operator=(RowViewBase other) requires std::is_same_v + { + static_cast&>(*this) = static_cast>(other); + return *this; + } + template RowViewBase(RowViewBase const& other) requires std::is_same_v { @@ -1427,6 +1455,12 @@ class Table *this = other; } + template + RowViewBase(RowViewBase other) requires std::is_same_v + { + *this = other; + } + RowViewBase& operator=(RowViewSentinel const& other) { this->mRowIndex = other.index; @@ -1586,7 +1620,7 @@ class Table // is held by the table, so we are safe passing the bare pointer. If it does it // means that the iterator on a table is outliving the table itself, which is // a bad idea. - return filtered_iterator(mColumnChunks, {selection, mOffset}); + return filtered_iterator(mColumnChunks, {selection, mTable->num_rows(), mOffset}); } iterator iteratorAt(uint64_t i) const @@ -1596,7 +1630,8 @@ class Table unfiltered_iterator rawIteratorAt(uint64_t i) const { - auto it = mBegin + i; + auto it = mBegin; + it.setCursor(i); return it; } @@ -2738,7 +2773,8 @@ struct Join : TableWrap::table_t { iterator rawIteratorAt(uint64_t i) const { - auto it = iterator{this->cached_begin()} + i; + auto it = iterator{this->cached_begin()}; + it.setCursor(i); return it; } @@ -2826,6 +2862,7 @@ class FilteredBase : public T using external_index_columns_t = typename T::external_index_columns_t; using iterator = decltype([](framework::pack) { return typename table_t::template RowViewFiltered, Os...>{}; }(originals{})); + using unfiltered_iterator = decltype([](framework::pack){ return typename table_t::template RowView, Os...>{}; }(originals{})); using const_iterator = iterator; FilteredBase(std::vector>&& tables, gandiva::Selection const& selection, uint64_t offset = 0) @@ -2873,6 +2910,13 @@ class FilteredBase : public T return const_iterator(mFilteredBegin); } + unfiltered_iterator rawIteratorAt(uint64_t i) const + { + auto it = unfiltered_iterator{mFilteredBegin}; + it.setCursor(i); + return it; + } + [[nodiscard]] RowViewSentinel end() const { return RowViewSentinel{*mFilteredEnd}; @@ -2908,6 +2952,19 @@ class FilteredBase : public T return mSelectedRows; } + auto rawSlice(uint64_t start, uint64_t end) const + { + auto s = this->asArrowTable()->Slice(start, end - start + 1); + SelectionVector newSelection{static_cast(end - start + 1)}; + std::iota(newSelection.begin(), newSelection.end(), 0); + return self_t{{s}, newSelection, start}; + } + + auto emptySlice() const + { + return self_t{{this->asArrowTable()}, SelectionVector{}, 0}; + } + static inline auto getSpan(gandiva::Selection const& sel) { if (sel == nullptr) { @@ -3033,6 +3090,11 @@ class FilteredBase : public T resetRanges(); } + bool isCached() const + { + return mCached; + } + private: void resetRanges() { @@ -3064,6 +3126,7 @@ class Filtered : public FilteredBase using originals = originals_pack_t; using iterator = decltype([](framework::pack) { return typename table_t::template RowViewFiltered, Os...>{}; }(originals{})); + using unfiltered_iterator = decltype([](framework::pack){ return typename table_t::template RowView, Os...>{}; }(originals{})); using const_iterator = iterator; iterator begin() @@ -3157,6 +3220,26 @@ class Filtered : public FilteredBase return operator*=(other.getSelectedRows()); } + unfiltered_iterator rawIteratorAt(uint64_t i) const + { + auto it = unfiltered_iterator{this->cached_begin()}; + it.setCursor(i); + return it; + } + + auto rawSlice(uint64_t start, uint64_t end) const + { + auto s = this->asArrowTable()->Slice(start, end - start + 1); + SelectionVector newSelection{static_cast(end - start + 1)}; + std::iota(newSelection.begin(), newSelection.end(), 0); + return self_t{{s}, newSelection, start}; + } + + auto emptySlice() const + { + return self_t{{this->asArrowTable()}, SelectionVector{}, 0}; + } + template auto rawSliceBy(o2::framework::Preslice const& container, int value) const { @@ -3200,6 +3283,7 @@ class Filtered> : public FilteredBase using table_t = typename FilteredBase::table_t; using originals = originals_pack_t; using iterator = decltype([](framework::pack) { return typename table_t::template RowViewFiltered>, Os...>{}; }(originals{})); + using unfiltered_iterator = decltype([](framework::pack){ return typename table_t::template RowView>, Os...>{}; }(originals{})); using const_iterator = iterator; iterator begin() @@ -3308,6 +3392,26 @@ class Filtered> : public FilteredBase return operator*=(other.getSelectedRows()); } + unfiltered_iterator rawIteratorAt(uint64_t i) const + { + auto it = unfiltered_iterator{this->cached_begin()}; + it.setCursor(i); + return it; + } + + auto rawSlice(uint64_t start, uint64_t end) const + { + auto s = this->asArrowTable()->Slice(start, end - start + 1); + SelectionVector newSelection{static_cast(end - start + 1)}; + std::iota(newSelection.begin(), newSelection.end(), 0); + return self_t{{s}, newSelection, start}; + } + + auto emptySlice() const + { + return self_t{{this->asArrowTable()}, SelectionVector{}, 0}; + } + auto sliceByCached(framework::expressions::BindingNode const& node, int value, o2::framework::SliceCache& cache) const { return doFilteredSliceByCached(this, node, value, cache); diff --git a/Framework/Core/include/Framework/AnalysisTask.h b/Framework/Core/include/Framework/AnalysisTask.h index 0e570a49c3ccf..dca4884e39691 100644 --- a/Framework/Core/include/Framework/AnalysisTask.h +++ b/Framework/Core/include/Framework/AnalysisTask.h @@ -349,9 +349,9 @@ struct AnalysisDataProcessorBuilder { auto associatedTables = AnalysisDataProcessorBuilder::bindAssociatedTables(inputs, processingFunction, infos); // pre-bind self indices std::apply( - [&](auto&... t) { + [&task](auto&... t) mutable { (homogeneous_apply_refs( - [&](auto& p) { + [&t](auto& p) { PartitionManager>::bindInternalIndices(p, &t); return true; }, @@ -360,9 +360,9 @@ struct AnalysisDataProcessorBuilder { }, associatedTables); - auto binder = [&](auto&& x) { + auto binder = [&task, &groupingTable, &associatedTables](auto& x) mutable { x.bindExternalIndices(&groupingTable, &std::get>(associatedTables)...); - homogeneous_apply_refs([&x](auto& t) { + homogeneous_apply_refs([&x](auto& t) mutable { PartitionManager>::setPartition(t, x); PartitionManager>::bindExternalIndices(t, &x); return true; @@ -373,7 +373,7 @@ struct AnalysisDataProcessorBuilder { // always pre-bind full tables to support index hierarchy std::apply( - [&](auto&&... x) { + [&binder](auto&... x) mutable { (binder(x), ...); }, associatedTables); @@ -391,7 +391,7 @@ struct AnalysisDataProcessorBuilder { auto associatedSlices = slice.associatedTables(); overwriteInternalIndices(associatedSlices, associatedTables); std::apply( - [&](auto&&... x) { + [&binder](auto&... x) mutable { (binder(x), ...); }, associatedSlices); diff --git a/Framework/Core/test/test_GroupSlicer.cxx b/Framework/Core/test/test_GroupSlicer.cxx index 3329ae4511436..7553871d27f69 100644 --- a/Framework/Core/test/test_GroupSlicer.cxx +++ b/Framework/Core/test/test_GroupSlicer.cxx @@ -9,8 +9,9 @@ // granted to it by virtue of its status as an Intergovernmental Organization // or submit itself to any jurisdiction. -#include "Framework/AnalysisTask.h" -#include "Framework/AnalysisDataModel.h" +#include "Framework/ASoA.h" +#include "Framework/TableBuilder.h" +#include "Framework/GroupSlicer.h" #include "Framework/ArrowTableSlicingCache.h" #include @@ -111,7 +112,7 @@ TEST_CASE("GroupSlicerOneAssociated") auto s = slices.updateCacheEntry(0, trkTable); o2::framework::GroupSlicer g(e, tt, slices); - unsigned int count = 0; + auto count = 0; for (auto& slice : g) { auto as = slice.associatedTables(); auto gg = slice.groupingElement(); @@ -189,7 +190,7 @@ TEST_CASE("GroupSlicerSeveralAssociated") s = slices.updateCacheEntry(2, {trkTableZ}); o2::framework::GroupSlicer g(e, tt, slices); - unsigned int count = 0; + auto count = 0; for (auto& slice : g) { auto as = slice.associatedTables(); auto gg = slice.groupingElement(); @@ -250,7 +251,7 @@ TEST_CASE("GroupSlicerMismatchedGroups") auto s = slices.updateCacheEntry(0, trkTable); o2::framework::GroupSlicer g(e, tt, slices); - unsigned int count = 0; + auto count = 0; for (auto& slice : g) { auto as = slice.associatedTables(); auto gg = slice.groupingElement(); @@ -308,7 +309,7 @@ TEST_CASE("GroupSlicerMismatchedUnassignedGroups") auto s = slices.updateCacheEntry(0, trkTable); o2::framework::GroupSlicer g(e, tt, slices); - unsigned int count = 0; + auto count = 0; for (auto& slice : g) { auto as = slice.associatedTables(); auto gg = slice.groupingElement(); @@ -358,7 +359,7 @@ TEST_CASE("GroupSlicerMismatchedFilteredGroups") auto s = slices.updateCacheEntry(0, trkTable); o2::framework::GroupSlicer g(e, tt, slices); - unsigned int count = 0; + auto count = 0; for (auto& slice : g) { auto as = slice.associatedTables(); @@ -471,6 +472,118 @@ TEST_CASE("GroupSlicerMismatchedUnsortedFilteredGroups") } } +namespace o2::aod +{ +namespace parts +{ +DECLARE_SOA_INDEX_COLUMN(Event, event); +DECLARE_SOA_COLUMN(Property, property, int); +DECLARE_SOA_SELF_SLICE_INDEX_COLUMN(Relatives, relatives); +} +DECLARE_SOA_TABLE(Parts, "AOD", "PRTS", soa::Index<>, parts::EventId, parts::Property, parts::RelativesIdSlice); + +namespace things +{ +DECLARE_SOA_INDEX_COLUMN(Event, event); +DECLARE_SOA_INDEX_COLUMN(Part, part); +} +DECLARE_SOA_TABLE(Things, "AOD", "THNGS", soa::Index<>, things::EventId, things::PartId); + +} + +template +static void overwriteInternalIndices(std::tuple& dest, std::tuple const& src) +{ + (std::get(dest).bindInternalIndicesTo(&std::get(src)), ...); +} + +TEST_CASE("GroupSlicerMismatchedUnsortedFilteredGroupsWithSelfIndex") +{ + TableBuilder builderE; + auto evtsWriter = builderE.cursor(); + for (auto i = 0; i < 20; ++i) { + evtsWriter(0, i, 0.5f * i, 2.f * i, 3.f * i); + } + auto evtTable = builderE.finalize(); + + TableBuilder builderP; + auto partsWriter = builderP.cursor(); + int filler[2]; + std::random_device rd; // a seed source for the random number engine + std::mt19937 gen(rd()); // mersenne_twister_engine seeded with rd() + std::uniform_int_distribution<> distrib(0, 99); + + for (auto i = 0; i < 100; ++i) { + + filler[0] = distrib(gen); + filler[1] = distrib(gen); + if (filler[0] > filler[1]) { + std::swap(filler[0], filler[1]); + } + partsWriter(0, std::floor(i/10.), i, filler); + } + auto partsTable = builderP.finalize(); + + TableBuilder builderT; + auto thingsWriter = builderT.cursor(); + for (auto i = 0; i < 10; ++i) { + thingsWriter(0, i, distrib(gen)); + } + auto thingsTable = builderT.finalize(); + + aod::Events e{evtTable}; + // aod::Parts p{partsTable}; + aod::Things t{thingsTable}; + using FilteredParts = soa::Filtered; + auto size = distrib(gen); + soa::SelectionVector rows; + for (auto i = 0; i < size; ++i) { + rows.push_back(distrib(gen)); + } + FilteredParts fp{{partsTable}, rows}; + auto associatedTuple = std::make_tuple(fp, t); + ArrowTableSlicingCache slices({{soa::getLabelFromType(), "fIndex" + o2::framework::cutString(soa::getLabelFromType())}, + {soa::getLabelFromType(), "fIndex" + o2::framework::cutString(soa::getLabelFromType())}}); + auto s0 = slices.updateCacheEntry(0, partsTable); + auto s1 = slices.updateCacheEntry(1, thingsTable); + o2::framework::GroupSlicer g(e, associatedTuple, slices); + + overwriteInternalIndices(associatedTuple, associatedTuple); + + // For a grouped case, the recursive access of a slice-self index of a filtered table should have consistent types + for (auto& slice : g) { + auto as = slice.associatedTables(); + auto gg = slice.groupingElement(); + overwriteInternalIndices(as, associatedTuple); + auto& ts = std::get<1>(as); + ts.bindExternalIndices(&e, &std::get<0>(associatedTuple)); + for (auto& thing : ts) { + if (thing.has_part()) { + auto part = thing.part_as(); + REQUIRE(std::is_same_v::parent_t,FilteredParts>); + auto rs = part.relatives_as>(); + REQUIRE(std::is_same_v, FilteredParts>); + for (auto& r : rs) { + REQUIRE(std::is_same_v::parent_t,FilteredParts>); + auto rss = r.relatives_as::parent_t>(); + REQUIRE(std::is_same_v, FilteredParts>); + for (auto& rr : rss) { + REQUIRE(std::is_same_v::parent_t,FilteredParts>); + auto rsss = rr.relatives_as::parent_t>(); + REQUIRE(std::is_same_v, FilteredParts>); + for (auto& rrr : rsss) { + REQUIRE(std::is_same_v::parent_t,FilteredParts>); + auto rssss = rrr.relatives_as::parent_t>(); + REQUIRE(std::is_same_v, FilteredParts>); + } + } + } + } + } + } + +} + TEST_CASE("EmptySliceables") { TableBuilder builderE; @@ -595,7 +708,7 @@ TEST_CASE("ArrowDirectSlicing") REQUIRE(arr[2] == 0.3f * (float)rid); auto d = row.lst(); - REQUIRE(d.size() == sizes[i]); + REQUIRE(d.size() == (size_t)sizes[i]); for (auto z = 0u; z < d.size(); ++z) { REQUIRE(d[z] == 0.5 * (double)z); } From 99d3f439fa6f908decfcbbc63b6cb64f5ebcff48 Mon Sep 17 00:00:00 2001 From: ALICE Action Bot Date: Tue, 5 Mar 2024 14:20:09 +0000 Subject: [PATCH 2/3] Please consider the following formatting changes --- Framework/Core/include/Framework/ASoA.h | 12 ++++++------ Framework/Core/test/test_GroupSlicer.cxx | 17 ++++++++--------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/Framework/Core/include/Framework/ASoA.h b/Framework/Core/include/Framework/ASoA.h index 4035d94fd433d..c459820f56d36 100644 --- a/Framework/Core/include/Framework/ASoA.h +++ b/Framework/Core/include/Framework/ASoA.h @@ -665,9 +665,9 @@ struct FilteredIndexPolicy : IndexPolicyBase { return O2_BUILTIN_UNLIKELY(mSelectionRow == sentinel.index); } - /// Move iterator to one after the end. Since this is a view - /// we move the mSelectionRow to one past the view size and - /// the mRowIndex to one past the last entry in the selection + /// Move iterator to one after the end. Since this is a view + /// we move the mSelectionRow to one past the view size and + /// the mRowIndex to one past the last entry in the selection void moveToEnd() { this->mSelectionRow = this->mMaxSelection; @@ -2862,7 +2862,7 @@ class FilteredBase : public T using external_index_columns_t = typename T::external_index_columns_t; using iterator = decltype([](framework::pack) { return typename table_t::template RowViewFiltered, Os...>{}; }(originals{})); - using unfiltered_iterator = decltype([](framework::pack){ return typename table_t::template RowView, Os...>{}; }(originals{})); + using unfiltered_iterator = decltype([](framework::pack) { return typename table_t::template RowView, Os...>{}; }(originals{})); using const_iterator = iterator; FilteredBase(std::vector>&& tables, gandiva::Selection const& selection, uint64_t offset = 0) @@ -3126,7 +3126,7 @@ class Filtered : public FilteredBase using originals = originals_pack_t; using iterator = decltype([](framework::pack) { return typename table_t::template RowViewFiltered, Os...>{}; }(originals{})); - using unfiltered_iterator = decltype([](framework::pack){ return typename table_t::template RowView, Os...>{}; }(originals{})); + using unfiltered_iterator = decltype([](framework::pack) { return typename table_t::template RowView, Os...>{}; }(originals{})); using const_iterator = iterator; iterator begin() @@ -3283,7 +3283,7 @@ class Filtered> : public FilteredBase using table_t = typename FilteredBase::table_t; using originals = originals_pack_t; using iterator = decltype([](framework::pack) { return typename table_t::template RowViewFiltered>, Os...>{}; }(originals{})); - using unfiltered_iterator = decltype([](framework::pack){ return typename table_t::template RowView>, Os...>{}; }(originals{})); + using unfiltered_iterator = decltype([](framework::pack) { return typename table_t::template RowView>, Os...>{}; }(originals{})); using const_iterator = iterator; iterator begin() diff --git a/Framework/Core/test/test_GroupSlicer.cxx b/Framework/Core/test/test_GroupSlicer.cxx index 7553871d27f69..82e92679d5e67 100644 --- a/Framework/Core/test/test_GroupSlicer.cxx +++ b/Framework/Core/test/test_GroupSlicer.cxx @@ -479,17 +479,17 @@ namespace parts DECLARE_SOA_INDEX_COLUMN(Event, event); DECLARE_SOA_COLUMN(Property, property, int); DECLARE_SOA_SELF_SLICE_INDEX_COLUMN(Relatives, relatives); -} +} // namespace parts DECLARE_SOA_TABLE(Parts, "AOD", "PRTS", soa::Index<>, parts::EventId, parts::Property, parts::RelativesIdSlice); namespace things { DECLARE_SOA_INDEX_COLUMN(Event, event); DECLARE_SOA_INDEX_COLUMN(Part, part); -} +} // namespace things DECLARE_SOA_TABLE(Things, "AOD", "THNGS", soa::Index<>, things::EventId, things::PartId); -} +} // namespace o2::aod template static void overwriteInternalIndices(std::tuple& dest, std::tuple const& src) @@ -520,7 +520,7 @@ TEST_CASE("GroupSlicerMismatchedUnsortedFilteredGroupsWithSelfIndex") if (filler[0] > filler[1]) { std::swap(filler[0], filler[1]); } - partsWriter(0, std::floor(i/10.), i, filler); + partsWriter(0, std::floor(i / 10.), i, filler); } auto partsTable = builderP.finalize(); @@ -560,19 +560,19 @@ TEST_CASE("GroupSlicerMismatchedUnsortedFilteredGroupsWithSelfIndex") for (auto& thing : ts) { if (thing.has_part()) { auto part = thing.part_as(); - REQUIRE(std::is_same_v::parent_t,FilteredParts>); + REQUIRE(std::is_same_v::parent_t, FilteredParts>); auto rs = part.relatives_as>(); REQUIRE(std::is_same_v, FilteredParts>); for (auto& r : rs) { - REQUIRE(std::is_same_v::parent_t,FilteredParts>); + REQUIRE(std::is_same_v::parent_t, FilteredParts>); auto rss = r.relatives_as::parent_t>(); REQUIRE(std::is_same_v, FilteredParts>); for (auto& rr : rss) { - REQUIRE(std::is_same_v::parent_t,FilteredParts>); + REQUIRE(std::is_same_v::parent_t, FilteredParts>); auto rsss = rr.relatives_as::parent_t>(); REQUIRE(std::is_same_v, FilteredParts>); for (auto& rrr : rsss) { - REQUIRE(std::is_same_v::parent_t,FilteredParts>); + REQUIRE(std::is_same_v::parent_t, FilteredParts>); auto rssss = rrr.relatives_as::parent_t>(); REQUIRE(std::is_same_v, FilteredParts>); } @@ -581,7 +581,6 @@ TEST_CASE("GroupSlicerMismatchedUnsortedFilteredGroupsWithSelfIndex") } } } - } TEST_CASE("EmptySliceables") From 8d7afd5bdbae38be2e739cb4b009a499e40c5ae7 Mon Sep 17 00:00:00 2001 From: Anton Alkin Date: Wed, 6 Mar 2024 11:16:42 +0100 Subject: [PATCH 3/3] fix temporary vector going out of scope; update tests --- Framework/Core/include/Framework/ASoA.h | 26 ++++++++++++---------- Framework/Core/test/test_ASoA.cxx | 29 +++++++++---------------- 2 files changed, 24 insertions(+), 31 deletions(-) diff --git a/Framework/Core/include/Framework/ASoA.h b/Framework/Core/include/Framework/ASoA.h index c459820f56d36..5c877a0e8f476 100644 --- a/Framework/Core/include/Framework/ASoA.h +++ b/Framework/Core/include/Framework/ASoA.h @@ -2954,10 +2954,10 @@ class FilteredBase : public T auto rawSlice(uint64_t start, uint64_t end) const { - auto s = this->asArrowTable()->Slice(start, end - start + 1); - SelectionVector newSelection{static_cast(end - start + 1)}; - std::iota(newSelection.begin(), newSelection.end(), 0); - return self_t{{s}, newSelection, start}; + SelectionVector newSelection; + newSelection.resize(static_cast(end - start + 1)); + std::iota(newSelection.begin(), newSelection.end(), start); + return self_t{{this->asArrowTable()}, std::move(newSelection), 0}; } auto emptySlice() const @@ -3227,12 +3227,14 @@ class Filtered : public FilteredBase return it; } + using FilteredBase::getSelectedRows; + auto rawSlice(uint64_t start, uint64_t end) const { - auto s = this->asArrowTable()->Slice(start, end - start + 1); - SelectionVector newSelection{static_cast(end - start + 1)}; - std::iota(newSelection.begin(), newSelection.end(), 0); - return self_t{{s}, newSelection, start}; + SelectionVector newSelection; + newSelection.resize(static_cast(end - start + 1)); + std::iota(newSelection.begin(), newSelection.end(), start); + return self_t{{this->asArrowTable()}, std::move(newSelection), 0}; } auto emptySlice() const @@ -3401,10 +3403,10 @@ class Filtered> : public FilteredBase auto rawSlice(uint64_t start, uint64_t end) const { - auto s = this->asArrowTable()->Slice(start, end - start + 1); - SelectionVector newSelection{static_cast(end - start + 1)}; - std::iota(newSelection.begin(), newSelection.end(), 0); - return self_t{{s}, newSelection, start}; + SelectionVector newSelection; + newSelection.resize(static_cast(end - start + 1)); + std::iota(newSelection.begin(), newSelection.end(), start); + return self_t{{this->asArrowTable()}, std::move(newSelection), 0}; } auto emptySlice() const diff --git a/Framework/Core/test/test_ASoA.cxx b/Framework/Core/test/test_ASoA.cxx index b97c359c6a739..d38c8183dc459 100644 --- a/Framework/Core/test/test_ASoA.cxx +++ b/Framework/Core/test/test_ASoA.cxx @@ -1039,20 +1039,16 @@ TEST_CASE("TestSelfIndexRecursion") // FIXME: only 4 levels of recursive self-index dereference are tested // self-index binding should stay the same for recursive dereferences for (auto& p : fp) { - auto bp = std::is_same_v, FullPoints::iterator>; - REQUIRE(bp); + REQUIRE(std::is_same_v, FullPoints::iterator>); auto ops = p.pointSeq_as(); for (auto& pp : ops) { - auto bpp = std::is_same_v, FullPoints::iterator>; - REQUIRE(bpp); + REQUIRE(std::is_same_v, FullPoints::iterator>); auto opps = pp.pointSeq_as(); for (auto& ppp : opps) { - auto bppp = std::is_same_v, FullPoints::iterator>; - REQUIRE(bppp); + REQUIRE(std::is_same_v, FullPoints::iterator>); auto oppps = ppp.pointSeq_as(); for (auto& pppp : oppps) { - auto bpppp = std::is_same_v, FullPoints::iterator>; - REQUIRE(bpppp); + REQUIRE(std::is_same_v, FullPoints::iterator>); auto opppps = pppp.pointSeq_as(); } } @@ -1077,21 +1073,16 @@ TEST_CASE("TestSelfIndexRecursion") // Filter should not interfere with self-index and the binding should stay the same for (auto& p : ffp) { - using T1 = std::decay_t; - auto bp = std::is_same_v; - REQUIRE(bp); - auto ops = p.pointSeq_as(); + REQUIRE(std::is_same_v, FilteredPoints::iterator>); + auto ops = p.pointSeq_as::parent_t>(); for (auto& pp : ops) { - auto bpp = std::is_same_v, FullPoints::iterator>; - REQUIRE(bpp); + REQUIRE(std::is_same_v::parent_t, FilteredPoints>); auto opps = pp.pointSeq_as(); for (auto& ppp : opps) { - auto bppp = std::is_same_v, FullPoints::iterator>; - REQUIRE(bppp); + REQUIRE(std::is_same_v, FilteredPoints::iterator>); auto oppps = ppp.pointSeq_as(); for (auto& pppp : oppps) { - auto bpppp = std::is_same_v, FullPoints::iterator>; - REQUIRE(bpppp); + REQUIRE(std::is_same_v, FilteredPoints::iterator>); auto opppps = pppp.pointSeq_as(); } } @@ -1100,7 +1091,7 @@ TEST_CASE("TestSelfIndexRecursion") auto const& ffpa = ffp; - // rawIteratorAt() should create an unfiltered iterator, unline begin() and iteratorAt() + // rawIteratorAt() should create an unfiltered iterator, unlike begin() and iteratorAt() for (auto& it1 : ffpa) { [[maybe_unused]] auto it2 = ffpa.rawIteratorAt(0); [[maybe_unused]] auto it3 = ffpa.iteratorAt(0);