From 72ee41340cdc46f3080730259cd5c08333a1d050 Mon Sep 17 00:00:00 2001 From: Bong-Hwi Lim Date: Sun, 25 Oct 2026 05:13:44 +0900 Subject: [PATCH 1/3] [Framework] Add opt-in skipping of empty grouped slices Filter empty associations before constructing typed slices while preserving the original grouping policy and windows. Keep IncludeEmpty as the default and document consumer and cache preconditions. Add Arrow-backed integration coverage for Pair and Triple policies, iterator copies, cache refresh, filtered grouping, bindings and window metadata. --- Framework/Core/CMakeLists.txt | 1 + .../include/Framework/GroupedCombinations.h | 78 ++++ .../Core/test/test_GroupedCombinations.cxx | 441 ++++++++++++++++++ 3 files changed, 520 insertions(+) create mode 100644 Framework/Core/test/test_GroupedCombinations.cxx diff --git a/Framework/Core/CMakeLists.txt b/Framework/Core/CMakeLists.txt index 437dc5ee6478c..3ee83abb779f9 100644 --- a/Framework/Core/CMakeLists.txt +++ b/Framework/Core/CMakeLists.txt @@ -239,6 +239,7 @@ add_executable(o2-test-framework-core test/test_FrameworkDataFlowToO2Control.cxx test/test_Graphviz.cxx test/test_GroupSlicer.cxx + test/test_GroupedCombinations.cxx test/test_HistogramRegistry.cxx test/test_HTTPParser.cxx test/test_IndexBuilder.cxx diff --git a/Framework/Core/include/Framework/GroupedCombinations.h b/Framework/Core/include/Framework/GroupedCombinations.h index d8c6aea44f31d..7c7f1a9ec5d05 100644 --- a/Framework/Core/include/Framework/GroupedCombinations.h +++ b/Framework/Core/include/Framework/GroupedCombinations.h @@ -21,6 +21,25 @@ namespace o2::framework { +/// Select which tuples of the original grouping policy are yielded. +/// IncludeEmpty preserves the existing stream, including empty associated slices. +/// SkipEmpty omits tuples with any empty association. Use it only when omitting +/// those tuples has no observable effect on the consumer, including counters, +/// normalization, window bookkeeping, random-number streams and diagnostics. +/// Window flags and neighbour counts retain their original-policy meaning: they +/// are not recomputed for surviving tuples, and a window may yield no tuple. +/// The grouping policy must terminate and advance without observable side effects; +/// an entirely empty association can end iteration without visiting every step. +enum class GroupedSlicePolicy { IncludeEmpty, + SkipEmpty }; + +template +struct SupportsEmptyAssociatedGate : std::false_type {}; + +template +struct SupportsEmptyAssociatedGate> + : std::bool_constant {}; + // Create an instance of a tuple interleaved from given tuples template auto interleaveTuplesImpl(std::tuple& t1, std::tuple& t2, std::index_sequence) @@ -67,6 +86,12 @@ struct GroupedCombinationsGenerator { : GroupingPolicy(groupingPolicy), mIndexColumns{getMatchingIndexNode()...}, cache{cache_} { } + GroupedIterator(const GroupingPolicy& groupingPolicy, SliceCache* cache_, GroupedSlicePolicy slicePolicy) + requires((SupportsEmptyAssociatedGate::value && ...)) + : GroupedIterator(groupingPolicy, cache_) + { + mSlicePolicy = slicePolicy; + } template GroupedIterator(const GroupingPolicy& groupingPolicy, const G& grouping, const std::tuple& associated, SliceCache* cache_) : GroupingPolicy(groupingPolicy), @@ -172,8 +197,42 @@ struct GroupedCombinationsGenerator { return std::get(*mAssociated).sliceByCached(mIndexColumns[I], ind, *cache); } + template + bool associatedHasRows() const + { + using A = std::tuple_element_t>; + auto const& associated = std::get(*mAssociated); + if (associated.size() == 0) { + return false; + } + auto const& node = mIndexColumns[I]; + auto localCache = cache->ptr->getCacheFor({"", soa::originReplacement(cache->ptr->newOrigin)(soa::getMatcherFromTypeForKey(node.name)), node.name}); + uint64_t index = *std::get<0>(std::get(this->mCurrent).getIndices()); + return localCache.getSliceFor(index).second != 0; + } + + template + bool allAssociatedHaveRows(std::index_sequence) const + { + return (associatedHasRows() && ...); + } + void setCurrentGroupedCombination() { + if constexpr ((SupportsEmptyAssociatedGate::value && ...)) { + if (mSlicePolicy == GroupedSlicePolicy::SkipEmpty && !this->mIsEnd) { + // A globally empty association makes every original pair ineligible. + if (std::apply([](auto const&... tables) { return ((tables.size() == 0) || ...); }, *mAssociated)) { + GroupingPolicy::moveToEnd(); + return; + } + // Advance the unchanged policy. Never remove grouping rows or refill + // a window with a more distant partner after rejecting an empty pair. + while (!this->mIsEnd && !allAssociatedHaveRows(std::index_sequence_for{})) { + this->addOne(); + } + } + } if (!this->mIsEnd) { auto& currentGrouping = GroupingPolicy::mCurrent; mSlices.emplace(getAssociatedTables()); @@ -187,6 +246,7 @@ struct GroupedCombinationsGenerator { std::optional> mSlices; std::optional mCurrentGrouped; SliceCache* cache = nullptr; + GroupedSlicePolicy mSlicePolicy = GroupedSlicePolicy::IncludeEmpty; }; using iterator = GroupedIterator; @@ -218,6 +278,24 @@ struct GroupedCombinationsGenerator { { setTables(grouping, associated); } + /// Opt in to the tuple filter described by GroupedSlicePolicy. + /// The type constraint accepts raw associated tables and excludes Join, Concat + /// and filtered associations. It does not validate sorting or cache contents. + /// As with sliceByCached, each matching association index must be sorted and + /// have an initialized sorted SliceCache entry for the current table/dataframe. + /// Grouping iterator indices must use the same coordinates as the cache keys. + /// The cache and bound tables must remain valid and unchanged during iteration. + /// Invalid/missing cache entries are not validated by this filter, and skipped + /// typed-slice construction need not reproduce diagnostics for invalid inputs. + template + GroupedCombinationsGenerator(const BP& binningPolicy, int catNeighbours, const T1& outsider, const G& grouping, const std::tuple& associated, SliceCache* cache, GroupedSlicePolicy slicePolicy) + requires((SupportsEmptyAssociatedGate::value && ...)) + : mBegin(GroupingPolicy(binningPolicy, catNeighbours, outsider), cache, slicePolicy), + mEnd(GroupingPolicy(binningPolicy, catNeighbours, outsider), cache, slicePolicy) + { + setTables(grouping, associated); + } + GroupedCombinationsGenerator(GroupedCombinationsGenerator const&) = default; GroupedCombinationsGenerator& operator=(GroupedCombinationsGenerator const&) = default; ~GroupedCombinationsGenerator() = default; diff --git a/Framework/Core/test/test_GroupedCombinations.cxx b/Framework/Core/test/test_GroupedCombinations.cxx new file mode 100644 index 0000000000000..0406ea306f75d --- /dev/null +++ b/Framework/Core/test/test_GroupedCombinations.cxx @@ -0,0 +1,441 @@ +// Copyright 2019-2026 CERN and copyright holders of ALICE O2. +// See https://alice-o2.web.cern.ch/copyright for details of the copyright holders. +// All rights not expressly granted are reserved. +// +// This software is distributed under the terms of the GNU General Public +// License v3 (GPL Version 3), copied verbatim in the file "COPYING". +// +// In applying this license CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// and does not submit itself to any jurisdiction. + +#include "Framework/ASoA.h" +#include "Framework/AnalysisDataModel.h" +#include "Framework/GroupedCombinations.h" +#include "Framework/TableBuilder.h" + +#include + +#include +#include +#include +#include +#include +#include + +using namespace o2::framework; +using namespace o2::soa; + +namespace o2::aod +{ +namespace groupedcombinations +{ +DECLARE_SOA_COLUMN(Category, category, int); +DECLARE_SOA_COLUMN(EventValue, eventValue, int); +DECLARE_SOA_COLUMN(DaughterValue, daughterValue, int); +} // namespace groupedcombinations +DECLARE_SOA_TABLE(GroupedEvents, "AOD", "GCEVENT", o2::soa::Index<>, groupedcombinations::Category, groupedcombinations::EventValue); +namespace groupedcombinations +{ +DECLARE_SOA_INDEX_COLUMN_FULL_CUSTOM(Event, event, int, GroupedEvents, "GCEVENT", ""); +} // namespace groupedcombinations +DECLARE_SOA_TABLE(GroupedLeftRows, "AOD", "GCLEFT", groupedcombinations::EventId, groupedcombinations::DaughterValue); +DECLARE_SOA_TABLE(GroupedRightRows, "AOD", "GCRIGHT", groupedcombinations::EventId, groupedcombinations::DaughterValue); +DECLARE_SOA_TABLE(GroupedThirdRows, "AOD", "GCTHIRD", groupedcombinations::EventId, groupedcombinations::DaughterValue); +} // namespace o2::aod + +namespace +{ +using namespace o2::aod::groupedcombinations; +using Events = o2::aod::GroupedEvents; +using LeftRows = o2::aod::GroupedLeftRows; +using RightRows = o2::aod::GroupedRightRows; +using ThirdRows = o2::aod::GroupedThirdRows; +using Binning = ColumnBinningPolicy; +using UpperPolicy = CombinationsBlockUpperSameIndexPolicy; +using FullPolicy = CombinationsBlockFullSameIndexPolicy; +using StrictPair = Pair; +using UpperPair = Pair; +using FullPair = Pair; +using StrictTriple = Triple; +using JoinedRows = o2::soa::Join; +using FilteredRows = o2::soa::Filtered; +using ConcatenatedRows = o2::soa::Concat; + +template +concept CanConstructSkipPair = requires(const Binning& binning, const Events& events, + const std::tuple& associated, + SliceCache* cache) { + Pair{binning, 1, -1, events, associated, cache, + GroupedSlicePolicy::SkipEmpty}; +}; + +static_assert(CanConstructSkipPair); +static_assert(!CanConstructSkipPair); +static_assert(!CanConstructSkipPair); +static_assert(!CanConstructSkipPair); +static_assert(!CanConstructSkipPair); + +struct Fixture { + std::shared_ptr eventsArrow; + std::shared_ptr leftArrow; + std::shared_ptr rightArrow; + std::shared_ptr thirdArrow; + Events events; + LeftRows left; + RightRows right; + ThirdRows third; + ArrowTableSlicingCache arrowCache; + SliceCache cache; + Binning binning{std::array, 1>{std::vector{VARIABLE_WIDTH, -0.5, 0.5, 1.5}}, true}; + + Fixture(std::vector leftIds = {0, 2, 3, 4}, + std::vector rightIds = {1, 2, 3, 5}, + std::vector thirdIds = {0, 1, 4, 5}) + : eventsArrow(makeEvents()), leftArrow(makeValues(leftIds, 200)), rightArrow(makeValues(rightIds, 300)), thirdArrow(makeValues(thirdIds, 400)), events(eventsArrow), left(leftArrow), right(rightArrow), third(thirdArrow), arrowCache(makeCache()), cache{&arrowCache} + { + left.bindExternalIndices(&events); + right.bindExternalIndices(&events); + third.bindExternalIndices(&events); + } + + template + static std::shared_ptr makeValues(const std::vector& ids, int base) + { + TableBuilder builder; + auto writer = builder.cursor(); + for (const auto id : ids) { + writer(0, id, base + id); + } + return builder.finalize(); + } + + static std::shared_ptr makeEvents() + { + TableBuilder builder; + auto writer = builder.cursor(); + for (const auto [category, value] : std::array, 7>{{{0, 100}, {0, 101}, {0, 102}, {1, 103}, {1, 104}, {1, 105}, {99, 199}}}) { + writer(0, category, value); + } + return builder.finalize(); + } + + ArrowTableSlicingCache makeCache() + { + const auto key = EventId::mLabel; + auto leftMatcher = getMatcherFromTypeForKey(key); + auto rightMatcher = getMatcherFromTypeForKey(key); + auto thirdMatcher = getMatcherFromTypeForKey(key); + ArrowTableSlicingCache result{{{getLabelFromType(), leftMatcher, key}, + {getLabelFromType(), rightMatcher, key}, + {getLabelFromType(), thirdMatcher, key}}, + {}, + o2::header::DataOrigin{"AOD"}}; + REQUIRE(result.updateCacheEntry(0, leftArrow).ok()); + REQUIRE(result.updateCacheEntry(1, rightArrow).ok()); + REQUIRE(result.updateCacheEntry(2, thirdArrow).ok()); + return result; + } +}; + +template +std::vector readPair(PairType& pairs, bool filterEmpty) +{ + std::vector values; + for (auto& [leftEvent, leftRows, rightEvent, rightRows] : pairs) { + if (filterEmpty && (leftRows.size() == 0 || rightRows.size() == 0)) { + continue; + } + REQUIRE(leftRows.size() > 0); + REQUIRE(rightRows.size() > 0); + REQUIRE(leftRows.begin().eventId() == leftEvent.globalIndex()); + REQUIRE(rightRows.begin().eventId() == rightEvent.globalIndex()); + REQUIRE(leftRows.begin().template event_as().eventValue() == 100 + leftEvent.globalIndex()); + REQUIRE(rightRows.begin().template event_as().eventValue() == 100 + rightEvent.globalIndex()); + values.push_back(std::to_string(leftEvent.globalIndex()) + ":" + std::to_string(rightEvent.globalIndex()) + ":" + + std::to_string(leftRows.begin().daughterValue()) + ":" + std::to_string(rightRows.begin().daughterValue())); + } + return values; +} + +TEST_CASE("Grouped combinations preserve default stream and skip empty slices on opt-in") +{ + Fixture fixture; + const auto associated = std::make_tuple(fixture.left, fixture.right); + const std::vector expectedStrictDepth1{"0:1:200:301", "4:5:204:305"}; + const std::vector expectedStrictDepth2{"0:1:200:301", "0:2:200:302", "3:5:203:305", "4:5:204:305"}; + const std::vector expectedStrictDepth10 = expectedStrictDepth2; + + for (const int depth : {1, 2, 10}) { + StrictPair baseline{fixture.binning, depth, -1, fixture.events, associated, &fixture.cache}; + const auto eligible = readPair(baseline, true); + REQUIRE(eligible == (depth == 1 ? expectedStrictDepth1 : depth == 2 ? expectedStrictDepth2 + : expectedStrictDepth10)); + + std::vector full; + for (auto& [first, left, second, right] : baseline) { + full.push_back(std::to_string(first.globalIndex()) + ":" + std::to_string(second.globalIndex()) + ":" + + std::to_string(left.size()) + ":" + std::to_string(right.size())); + } + REQUIRE(full.size() == (depth == 1 ? 4 : 6)); + REQUIRE(full.front() == "0:1:1:1"); + REQUIRE(full.back() == "4:5:1:1"); + + StrictPair optimized{fixture.binning, depth, -1, fixture.events, associated, &fixture.cache, + GroupedSlicePolicy::SkipEmpty}; + REQUIRE(readPair(optimized, false) == eligible); + + if (depth == 1) { + auto iterator = baseline.begin(); + auto copy = iterator; + auto previous = iterator++; + REQUIRE(std::get<0>(*previous).globalIndex() == 0); + REQUIRE(std::get<0>(*iterator).globalIndex() == 1); + REQUIRE(std::get<0>(*copy).globalIndex() == 0); + auto advancedCopy = iterator; + REQUIRE(std::get<0>(*advancedCopy).globalIndex() == 1); + + auto optimizedIterator = optimized.begin(); + auto independent = optimizedIterator; + auto saved = optimizedIterator++; + REQUIRE(std::get<0>(*saved).globalIndex() == 0); + REQUIRE(std::get<2>(*saved).globalIndex() == 1); + REQUIRE(std::get<0>(*optimizedIterator).globalIndex() == 4); + REQUIRE(std::get<0>(*saved).globalIndex() == 0); + REQUIRE(std::get<0>(*independent).globalIndex() == 0); + auto advancedOptimizedCopy = optimizedIterator; + ++optimizedIterator; + REQUIRE(optimizedIterator == optimized.end()); + REQUIRE(std::get<0>(*advancedOptimizedCopy).globalIndex() == 4); + REQUIRE(std::get<1>(*advancedOptimizedCopy).begin().template event_as().eventValue() == 104); + REQUIRE(std::get<0>(*independent).globalIndex() == 0); + ++advancedOptimizedCopy; + REQUIRE(advancedOptimizedCopy == optimized.end()); + } + } + + // The seven-event data frame has two populated categories and one outsider. + // No combination may cross the category boundary or include the outsider. + UpperPair upper{fixture.binning, 2, -1, fixture.events, associated, &fixture.cache}; + const auto upperEligible = readPair(upper, true); + REQUIRE(upperEligible == (std::vector{"0:1:200:301", "0:2:200:302", "2:2:202:302", "3:3:203:303", "3:5:203:305", "4:5:204:305"})); + UpperPair upperOptimized{fixture.binning, 2, -1, fixture.events, associated, &fixture.cache, + GroupedSlicePolicy::SkipEmpty}; + REQUIRE(readPair(upperOptimized, false) == upperEligible); + for (const auto& tuple : upperEligible) { + REQUIRE(tuple.find("6:") != 0); + REQUIRE(tuple.find(":6:") == std::string::npos); + } + FullPair full{fixture.binning, 2, -1, fixture.events, associated, &fixture.cache}; + const auto fullEligible = readPair(full, true); + REQUIRE(fullEligible == (std::vector{"0:1:200:301", "0:2:200:302", "2:1:202:301", "2:2:202:302", "3:3:203:303", "3:5:203:305", "4:3:204:303", "4:5:204:305"})); + FullPair fullOptimized{fixture.binning, 2, -1, fixture.events, associated, &fixture.cache, + GroupedSlicePolicy::SkipEmpty}; + REQUIRE(readPair(fullOptimized, false) == fullEligible); +} + +TEST_CASE("Grouped triple skips if any associated slice is empty") +{ + Fixture fixture; + const auto associated = std::make_tuple(fixture.left, fixture.right, fixture.third); + StrictTriple baseline{fixture.binning, 10, -1, fixture.events, associated, &fixture.cache}; + std::vector eligible; + std::vector full; + for (auto& [first, left, second, right, thirdEvent, third] : baseline) { + const auto tuple = std::to_string(first.globalIndex()) + ":" + std::to_string(second.globalIndex()) + ":" + + std::to_string(thirdEvent.globalIndex()); + full.push_back(tuple); + if (left.size() && right.size() && third.size()) { + eligible.push_back(tuple); + REQUIRE(left.begin().eventId() == first.globalIndex()); + REQUIRE(right.begin().eventId() == second.globalIndex()); + REQUIRE(third.begin().eventId() == thirdEvent.globalIndex()); + REQUIRE(third.begin().daughterValue() == 400 + thirdEvent.globalIndex()); + } + } + REQUIRE(full.size() == 2); + REQUIRE(eligible.empty()); + StrictTriple optimized{fixture.binning, 10, -1, fixture.events, associated, &fixture.cache, + GroupedSlicePolicy::SkipEmpty}; + size_t count = 0; + for (auto& tuple : optimized) { + (void)tuple; + ++count; + } + REQUIRE(count == eligible.size()); +} + +TEST_CASE("Grouped combinations handle empty and singleton strict-upper tables") +{ + Fixture fixture; + TableBuilder emptyBuilder; + auto emptyWriter = emptyBuilder.cursor(); + (void)emptyWriter; + Events empty{emptyBuilder.finalize()}; + const auto associated = std::make_tuple(fixture.left, fixture.right); + StrictPair emptyDefault{fixture.binning, 10, -1, empty, associated, &fixture.cache}; + REQUIRE(emptyDefault.begin() == emptyDefault.end()); + StrictPair emptySkip{fixture.binning, 10, -1, empty, associated, &fixture.cache, GroupedSlicePolicy::SkipEmpty}; + REQUIRE(emptySkip.begin() == emptySkip.end()); + + TableBuilder singletonBuilder; + auto singletonWriter = singletonBuilder.cursor(); + singletonWriter(0, 0, 700); + Events singleton{singletonBuilder.finalize()}; + auto left = fixture.left; + auto right = fixture.right; + left.bindExternalIndices(&singleton); + right.bindExternalIndices(&singleton); + const auto singletonAssociated = std::make_tuple(left, right); + StrictPair singletonDefault{fixture.binning, 10, -1, singleton, singletonAssociated, &fixture.cache}; + REQUIRE(singletonDefault.begin() == singletonDefault.end()); + StrictPair singletonSkip{fixture.binning, 10, -1, singleton, singletonAssociated, &fixture.cache, + GroupedSlicePolicy::SkipEmpty}; + REQUIRE(singletonSkip.begin() == singletonSkip.end()); + + // A generator that saw an empty data frame must initialize a fresh stream + // when setTables supplies populated input later. + StrictPair reused{fixture.binning, 2, -1, empty, associated, &fixture.cache}; + REQUIRE(reused.begin() == reused.end()); + reused.setTables(fixture.events, associated); + REQUIRE(readPair(reused, true) == (std::vector{"0:1:200:301", "0:2:200:302", "3:5:203:305", "4:5:204:305"})); + StrictPair reusedSkip{fixture.binning, 2, -1, empty, associated, &fixture.cache, GroupedSlicePolicy::SkipEmpty}; + reusedSkip.setTables(fixture.events, associated); + REQUIRE(readPair(reusedSkip, false) == readPair(reused, true)); +} + +template +std::vector> readTriple(TripleType& triples, bool filterEmpty) +{ + std::vector> values; + for (auto& [first, left, second, right, thirdEvent, third] : triples) { + if (filterEmpty && (left.size() == 0 || right.size() == 0 || third.size() == 0)) { + continue; + } + REQUIRE(left.size() == 1); + REQUIRE(right.size() == 1); + REQUIRE(third.size() == 1); + REQUIRE(left.begin().daughterValue() == 200 + first.globalIndex()); + REQUIRE(right.begin().daughterValue() == 300 + second.globalIndex()); + REQUIRE(third.begin().daughterValue() == 400 + thirdEvent.globalIndex()); + REQUIRE(left.begin().template event_as().eventValue() == first.eventValue()); + REQUIRE(right.begin().template event_as().eventValue() == second.eventValue()); + REQUIRE(third.begin().template event_as().eventValue() == thirdEvent.eventValue()); + values.push_back({first.globalIndex(), second.globalIndex(), thirdEvent.globalIndex()}); + } + return values; +} + +TEST_CASE("Grouped triples preserve policy streams and reject each empty position") +{ + using Upper = CombinationsBlockUpperSameIndexPolicy; + using Full = CombinationsBlockFullSameIndexPolicy; + using UpperTriple = Triple; + using FullTriple = Triple; + const std::vector populated{0, 1, 2, 3, 4, 5}; + const std::vector> expectedAll{{0, 1, 2}, {3, 4, 5}}; + const std::vector> expectedOne{{3, 4, 5}}; + for (int missing = -1; missing < 3; ++missing) { + auto left = populated; + auto right = populated; + auto third = populated; + if (missing == 0) { + left.erase(left.begin()); + } + if (missing == 1) { + right.erase(right.begin() + 1); + } + if (missing == 2) { + third.erase(third.begin() + 2); + } + Fixture fixture{left, right, third}; + const auto associated = std::make_tuple(fixture.left, fixture.right, fixture.third); + StrictTriple baseline{fixture.binning, 10, -1, fixture.events, associated, &fixture.cache}; + StrictTriple optimized{fixture.binning, 10, -1, fixture.events, associated, &fixture.cache, + GroupedSlicePolicy::SkipEmpty}; + const auto expected = missing < 0 ? expectedAll : expectedOne; + REQUIRE(readTriple(baseline, true) == expected); + REQUIRE(readTriple(optimized, false) == expected); + auto first = baseline.begin(); + auto& tuple = *first; + REQUIRE(std::get<1>(tuple).size() == (missing == 0 ? 0 : 1)); + REQUIRE(std::get<3>(tuple).size() == (missing == 1 ? 0 : 1)); + REQUIRE(std::get<5>(tuple).size() == (missing == 2 ? 0 : 1)); + + UpperTriple upper{fixture.binning, 2, -1, fixture.events, associated, &fixture.cache}; + UpperTriple upperSkip{fixture.binning, 2, -1, fixture.events, associated, &fixture.cache, + GroupedSlicePolicy::SkipEmpty}; + const auto upperRows = readTriple(upper, true); + REQUIRE(readTriple(upperSkip, false) == upperRows); + FullTriple full{fixture.binning, 2, -1, fixture.events, associated, &fixture.cache}; + FullTriple fullSkip{fixture.binning, 2, -1, fixture.events, associated, &fixture.cache, + GroupedSlicePolicy::SkipEmpty}; + const auto fullRows = readTriple(full, true); + REQUIRE(readTriple(fullSkip, false) == fullRows); + if (missing < 0) { + REQUIRE(upperRows.size() == 20); + REQUIRE(fullRows.size() == 54); + REQUIRE(upperRows.front() == (std::array{0, 0, 0})); + REQUIRE(upperRows.back() == (std::array{5, 5, 5})); + REQUIRE(fullRows.front() == (std::array{0, 0, 0})); + REQUIRE(fullRows.back() == (std::array{5, 5, 5})); + } + } +} + +TEST_CASE("Grouped skip policy survives an empty associated dataframe") +{ + Fixture fixture; + auto emptyArrow = Fixture::makeValues({}, 200); + LeftRows empty{emptyArrow}; + empty.bindExternalIndices(&fixture.events); + REQUIRE(fixture.arrowCache.updateCacheEntry(0, emptyArrow).ok()); + StrictPair reused{fixture.binning, 2, -1, fixture.events, std::make_tuple(empty, fixture.right), + &fixture.cache, GroupedSlicePolicy::SkipEmpty}; + REQUIRE(reused.begin() == reused.end()); + REQUIRE(fixture.arrowCache.updateCacheEntry(0, fixture.leftArrow).ok()); + reused.setTables(fixture.events, std::make_tuple(fixture.left, fixture.right)); + REQUIRE(readPair(reused, false) == (std::vector{"0:1:200:301", "0:2:200:302", "3:5:203:305", "4:5:204:305"})); +} + +template +std::vector> readWindowState(PairType& pairs, bool filterEmpty) +{ + std::vector> states; + for (auto iterator = pairs.begin(); iterator != pairs.end(); ++iterator) { + auto& [first, left, second, right] = *iterator; + if (filterEmpty && (left.size() == 0 || right.size() == 0)) { + continue; + } + states.push_back({first.globalIndex(), second.globalIndex(), iterator.isNewWindow(), iterator.currentWindowNeighbours()}); + } + return states; +} + +TEST_CASE("Grouped skip retains original window metadata") +{ + Fixture fixture{{0}, {2}, {0, 1, 2}}; + const auto associated = std::make_tuple(fixture.left, fixture.right); + StrictPair baseline{fixture.binning, 2, -1, fixture.events, associated, &fixture.cache}; + StrictPair optimized{fixture.binning, 2, -1, fixture.events, associated, &fixture.cache, + GroupedSlicePolicy::SkipEmpty}; + const std::vector> expected{{0, 2, 0, 2}}; + REQUIRE(readWindowState(baseline, true) == expected); + REQUIRE(readWindowState(optimized, false) == expected); +} + +TEST_CASE("Grouped skip uses original indices for filtered grouping rows") +{ + Fixture fixture; + const auto associated = std::make_tuple(fixture.left, fixture.right); + using SelectedEvents = o2::soa::Filtered; + using SelectedPair = Pair; + SelectedEvents selected{{fixture.events.asArrowTableRef()}, SelectionVector{0, 2, 3, 5}}; + SelectedPair selectedDefault{fixture.binning, 10, -1, selected, associated, &fixture.cache}; + SelectedPair selectedSkip{fixture.binning, 10, -1, selected, associated, &fixture.cache, + GroupedSlicePolicy::SkipEmpty}; + const std::vector expectedSelected{"0:2:200:302", "3:5:203:305"}; + REQUIRE(readPair(selectedDefault, true) == expectedSelected); + REQUIRE(readPair(selectedSkip, false) == expectedSelected); +} +} // namespace From 5f7caad3735e3080e6c8eb4e9c671aad390bf6f4 Mon Sep 17 00:00:00 2001 From: Bong-Hwi Lim Date: Sun, 25 Oct 2026 13:27:49 +0900 Subject: [PATCH 2/3] [Framework] Trim comments from grouped-slice update Remove the explanatory comments added with the SkipEmpty policy and its integration coverage. Keep existing comments, license headers and code unchanged. --- .../include/Framework/GroupedCombinations.h | 21 ------------------- .../Core/test/test_GroupedCombinations.cxx | 4 ---- 2 files changed, 25 deletions(-) diff --git a/Framework/Core/include/Framework/GroupedCombinations.h b/Framework/Core/include/Framework/GroupedCombinations.h index 7c7f1a9ec5d05..aba1d22a628ae 100644 --- a/Framework/Core/include/Framework/GroupedCombinations.h +++ b/Framework/Core/include/Framework/GroupedCombinations.h @@ -21,15 +21,6 @@ namespace o2::framework { -/// Select which tuples of the original grouping policy are yielded. -/// IncludeEmpty preserves the existing stream, including empty associated slices. -/// SkipEmpty omits tuples with any empty association. Use it only when omitting -/// those tuples has no observable effect on the consumer, including counters, -/// normalization, window bookkeeping, random-number streams and diagnostics. -/// Window flags and neighbour counts retain their original-policy meaning: they -/// are not recomputed for surviving tuples, and a window may yield no tuple. -/// The grouping policy must terminate and advance without observable side effects; -/// an entirely empty association can end iteration without visiting every step. enum class GroupedSlicePolicy { IncludeEmpty, SkipEmpty }; @@ -221,13 +212,10 @@ struct GroupedCombinationsGenerator { { if constexpr ((SupportsEmptyAssociatedGate::value && ...)) { if (mSlicePolicy == GroupedSlicePolicy::SkipEmpty && !this->mIsEnd) { - // A globally empty association makes every original pair ineligible. if (std::apply([](auto const&... tables) { return ((tables.size() == 0) || ...); }, *mAssociated)) { GroupingPolicy::moveToEnd(); return; } - // Advance the unchanged policy. Never remove grouping rows or refill - // a window with a more distant partner after rejecting an empty pair. while (!this->mIsEnd && !allAssociatedHaveRows(std::index_sequence_for{})) { this->addOne(); } @@ -278,15 +266,6 @@ struct GroupedCombinationsGenerator { { setTables(grouping, associated); } - /// Opt in to the tuple filter described by GroupedSlicePolicy. - /// The type constraint accepts raw associated tables and excludes Join, Concat - /// and filtered associations. It does not validate sorting or cache contents. - /// As with sliceByCached, each matching association index must be sorted and - /// have an initialized sorted SliceCache entry for the current table/dataframe. - /// Grouping iterator indices must use the same coordinates as the cache keys. - /// The cache and bound tables must remain valid and unchanged during iteration. - /// Invalid/missing cache entries are not validated by this filter, and skipped - /// typed-slice construction need not reproduce diagnostics for invalid inputs. template GroupedCombinationsGenerator(const BP& binningPolicy, int catNeighbours, const T1& outsider, const G& grouping, const std::tuple& associated, SliceCache* cache, GroupedSlicePolicy slicePolicy) requires((SupportsEmptyAssociatedGate::value && ...)) diff --git a/Framework/Core/test/test_GroupedCombinations.cxx b/Framework/Core/test/test_GroupedCombinations.cxx index 0406ea306f75d..a061052da3f97 100644 --- a/Framework/Core/test/test_GroupedCombinations.cxx +++ b/Framework/Core/test/test_GroupedCombinations.cxx @@ -214,8 +214,6 @@ TEST_CASE("Grouped combinations preserve default stream and skip empty slices on } } - // The seven-event data frame has two populated categories and one outsider. - // No combination may cross the category boundary or include the outsider. UpperPair upper{fixture.binning, 2, -1, fixture.events, associated, &fixture.cache}; const auto upperEligible = readPair(upper, true); REQUIRE(upperEligible == (std::vector{"0:1:200:301", "0:2:200:302", "2:2:202:302", "3:3:203:303", "3:5:203:305", "4:5:204:305"})); @@ -293,8 +291,6 @@ TEST_CASE("Grouped combinations handle empty and singleton strict-upper tables") GroupedSlicePolicy::SkipEmpty}; REQUIRE(singletonSkip.begin() == singletonSkip.end()); - // A generator that saw an empty data frame must initialize a fresh stream - // when setTables supplies populated input later. StrictPair reused{fixture.binning, 2, -1, empty, associated, &fixture.cache}; REQUIRE(reused.begin() == reused.end()); reused.setTables(fixture.events, associated); From 06cc233e87c9d4b06dd7d4f2eedbb472fb0e3d08 Mon Sep 17 00:00:00 2001 From: Bong-Hwi Lim Date: Sun, 25 Oct 2026 13:44:42 +0900 Subject: [PATCH 3/3] [Framework] Fix grouped-combination copyright and formatting Use the canonical ALICE O2 copyright wording in the integration test. Align the empty eligibility structs with clang-format 20. --- Framework/Core/include/Framework/GroupedCombinations.h | 6 ++++-- Framework/Core/test/test_GroupedCombinations.cxx | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/Framework/Core/include/Framework/GroupedCombinations.h b/Framework/Core/include/Framework/GroupedCombinations.h index aba1d22a628ae..509e5a66faea0 100644 --- a/Framework/Core/include/Framework/GroupedCombinations.h +++ b/Framework/Core/include/Framework/GroupedCombinations.h @@ -25,11 +25,13 @@ enum class GroupedSlicePolicy { IncludeEmpty, SkipEmpty }; template -struct SupportsEmptyAssociatedGate : std::false_type {}; +struct SupportsEmptyAssociatedGate : std::false_type { +}; template struct SupportsEmptyAssociatedGate> - : std::bool_constant {}; + : std::bool_constant { +}; // Create an instance of a tuple interleaved from given tuples template diff --git a/Framework/Core/test/test_GroupedCombinations.cxx b/Framework/Core/test/test_GroupedCombinations.cxx index a061052da3f97..54bbc6efab1cd 100644 --- a/Framework/Core/test/test_GroupedCombinations.cxx +++ b/Framework/Core/test/test_GroupedCombinations.cxx @@ -7,7 +7,7 @@ // // In applying this license CERN does not waive the privileges and immunities // granted to it by virtue of its status as an Intergovernmental Organization -// and does not submit itself to any jurisdiction. +// or submit itself to any jurisdiction. #include "Framework/ASoA.h" #include "Framework/AnalysisDataModel.h"