From b3c384fd34a3f90d017e0926a4912310b781b1be Mon Sep 17 00:00:00 2001 From: RobinTF <83676088+RobinTF@users.noreply.github.com> Date: Wed, 7 Jan 2026 12:09:05 +0100 Subject: [PATCH 1/3] Properly differentiate between cancelled and failed for lazy results --- src/engine/Operation.cpp | 16 ++++++++++----- src/engine/Operation.h | 1 + src/engine/Result.cpp | 21 ++++++++++++-------- src/engine/Result.h | 13 ++++++++---- test/OperationTest.cpp | 33 ++++++++++++++++++++++++++++++ test/ResultTest.cpp | 43 +++++++++++++++++++++++++++++++++------- 6 files changed, 103 insertions(+), 24 deletions(-) diff --git a/src/engine/Operation.cpp b/src/engine/Operation.cpp index 2d313ae937..26697306da 100644 --- a/src/engine/Operation.cpp +++ b/src/engine/Operation.cpp @@ -208,11 +208,17 @@ Result Operation::runComputation(const ad_utility::Timer& timer, } signalQueryUpdate(RuntimeInformation::SendPriority::IfDue); }, - [this](bool failed) { - // TODO Distinguish between failed and cancelled. - runtimeInfo().status_ = - failed ? RuntimeInformation::failed - : RuntimeInformation::lazilyMaterializedCompleted; + [this](Result::GeneratorState state) { + runtimeInfo().status_ = [state]() { + switch (state) { + case Result::GeneratorState::FINISHED: + return RuntimeInformation::lazilyMaterializedCompleted; + case Result::GeneratorState::CANCELLED: + return RuntimeInformation::cancelled; + case Result::GeneratorState::FAILED: + return RuntimeInformation::failed; + } + }(); signalQueryUpdate(RuntimeInformation::SendPriority::Always); }); } diff --git a/src/engine/Operation.h b/src/engine/Operation.h index 58ea7ae833..d60fd240e6 100644 --- a/src/engine/Operation.h +++ b/src/engine/Operation.h @@ -520,6 +520,7 @@ class Operation { FRIEND_TEST(Operation, updateRuntimeStatsWorksCorrectly); FRIEND_TEST(Operation, verifyRuntimeInformationIsUpdatedForLazyOperations); FRIEND_TEST(Operation, ensureFailedStatusIsSetWhenGeneratorThrowsException); + FRIEND_TEST(Operation, ensureFailedStatusIsSetWhenGeneratorIsCancelled); FRIEND_TEST(Operation, testSubMillisecondsIncrementsAreStillTracked); FRIEND_TEST(Operation, ensureSignalUpdateIsOnlyCalledEvery50msAndAtTheEnd); FRIEND_TEST(Operation, diff --git a/src/engine/Result.cpp b/src/engine/Result.cpp index 1a63d628da..d8abcfeda4 100644 --- a/src/engine/Result.cpp +++ b/src/engine/Result.cpp @@ -270,7 +270,7 @@ void Result::checkDefinedness(const VariableToColumnMap& varColMap) { void Result::runOnNewChunkComputed( std::function onNewChunk, - std::function onGeneratorFinished) { + std::function onGeneratorFinished) { AD_CONTRACT_CHECK(!isFullyMaterialized()); auto inputAsGet = ad_utility::CachingTransformInputRange( idTables(), [](auto& input) { return std::move(input); }); @@ -281,24 +281,29 @@ void Result::runOnNewChunkComputed( std::move(onGeneratorFinished)); // The main lambda that when being called processes the next chunk. - auto get = - [inputAsGet = std::move(inputAsGet), sharedFinish, - cleanup = absl::Cleanup{[&finish = *sharedFinish]() { finish(false); }}, - onNewChunk = - std::move(onNewChunk)]() mutable -> std::optional { + auto get = [inputAsGet = std::move(inputAsGet), sharedFinish, + cleanup = absl::Cleanup{[&finish = *sharedFinish]() { + finish(GeneratorState::FINISHED); + }}, + onNewChunk = std::move( + onNewChunk)]() mutable -> std::optional { try { Timer timer{Timer::Started}; auto input = inputAsGet.get(); if (!input.has_value()) { std::move(cleanup).Cancel(); - (*sharedFinish)(false); + (*sharedFinish)(GeneratorState::FINISHED); return std::nullopt; } onNewChunk(input.value(), timer.value()); return input; + } catch (const ad_utility::CancellationException&) { + std::move(cleanup).Cancel(); + (*sharedFinish)(GeneratorState::CANCELLED); + throw; } catch (...) { std::move(cleanup).Cancel(); - (*sharedFinish)(true); + (*sharedFinish)(GeneratorState::FAILED); throw; } }; diff --git a/src/engine/Result.h b/src/engine/Result.h index 87a2b85e78..63525ae18c 100644 --- a/src/engine/Result.h +++ b/src/engine/Result.h @@ -38,6 +38,9 @@ class Result { : idTable_{std::move(idTable)}, localVocab_{std::move(localVocab)} {} }; + // Helper enum to indicate the state of a generator after consumption. + enum class GeneratorState { FINISHED, CANCELLED, FAILED }; + // The lazy result type that is actually stored. It is type-erased and allows // explicit conversion from the `Generator` above. using LazyResult = ad_utility::InputRangeTypeErased; @@ -160,16 +163,18 @@ class Result { // generator and passed this new `IdTableVocabPair` along with microsecond // precision timing information on how long it took to compute this new chunk. // `onGeneratorFinished` is guaranteed to be called eventually as long as the - // generator is consumed at least partially, with `true` if an exception - // occurred during consumption or with `false` when the generator is done - // processing or abandoned and destroyed. + // generator is consumed at least partially, with `GeneratorState::FAILED` if + // an exception occurred during consumption, with `GeneratorState::CANCELLED` + // if said exception is a cancellation exception or with + // `GeneratorState::FINISHED` when the generator is done processing or + // abandoned and destroyed. // // Throw an `ad_utility::Exception` if the underlying `data_` member holds the // wrong variant. void runOnNewChunkComputed( std::function onNewChunk, - std::function onGeneratorFinished); + std::function onGeneratorFinished); // Wrap the generator stored in `data_` within a new generator that aggregates // the entries yielded by the generator into a cacheable `IdTable`. Once diff --git a/test/OperationTest.cpp b/test/OperationTest.cpp index 4b74b6ae9e..15ff985dff 100644 --- a/test/OperationTest.cpp +++ b/test/OperationTest.cpp @@ -508,6 +508,39 @@ TEST(Operation, ensureFailedStatusIsSetWhenGeneratorThrowsException) { EXPECT_TRUE(signaledUpdate); } +// _____________________________________________________________________________ +TEST(Operation, ensureFailedStatusIsSetWhenGeneratorIsCancelled) { + bool signaledUpdate = false; + const Index& index = ad_utility::testing::getQec()->getIndex(); + QueryResultCache cache{}; + NamedResultCache namedCache{}; + MaterializedViewsManager materializedViewsManager; + QueryExecutionContext context{ + index, + &cache, + makeAllocator(ad_utility::MemorySize::megabytes(100)), + SortPerformanceEstimator{}, + &namedCache, + &materializedViewsManager, + [&](std::string) { signaledUpdate = true; }}; + CustomGeneratorOperation operation{ + &context, []() -> Result::Generator { + throw CancellationException{"Operation was cancelled"}; + co_return; + }()}; + ad_utility::Timer timer{ad_utility::Timer::InitialStatus::Started}; + auto result = + operation.runComputation(timer, ComputationMode::LAZY_IF_SUPPORTED); + + EXPECT_EQ(operation.runtimeInfo().status_, + Status::lazilyMaterializedInProgress); + + EXPECT_THROW(result.idTables().begin(), ad_utility::CancellationException); + + EXPECT_EQ(operation.runtimeInfo().status_, Status::cancelled); + EXPECT_TRUE(signaledUpdate); +} + // _____________________________________________________________________________ TEST(Operation, ensureSignalUpdateIsOnlyCalledEvery50msAndAtTheEnd) { #ifdef _QLEVER_NO_TIMING_TESTS diff --git a/test/ResultTest.cpp b/test/ResultTest.cpp index e40bf9ca9f..0c18b8a786 100644 --- a/test/ResultTest.cpp +++ b/test/ResultTest.cpp @@ -163,7 +163,7 @@ TEST(Result, verifyRunOnNewChunkComputedThrowsWithFullyMaterializedResult) { EXPECT_THROW(result.runOnNewChunkComputed( [](const IdTableVocabPair&, std::chrono::microseconds) {}, - [](bool) {}), + [](Result::GeneratorState) {}), ad_utility::Exception); } @@ -208,8 +208,8 @@ TEST(Result, verifyRunOnNewChunkComputedFiresCorrectly) { EXPECT_GE(duration, 5ms); } }, - [&](bool error) { - EXPECT_FALSE(error); + [&](Result::GeneratorState state) { + EXPECT_EQ(state, Result::GeneratorState::FINISHED); finishedConsuming = true; }); @@ -234,8 +234,8 @@ TEST(Result, verifyRunOnNewChunkCallsFinishOnError) { [&](const IdTableVocabPair&, std::chrono::microseconds) { ++callCounterGenerator; }, - [&](bool error) { - EXPECT_TRUE(error); + [&](Result::GeneratorState state) { + EXPECT_EQ(state, Result::GeneratorState::FAILED); ++callCounterFinished; }); @@ -247,6 +247,35 @@ TEST(Result, verifyRunOnNewChunkCallsFinishOnError) { EXPECT_EQ(callCounterFinished, 1); } +// _____________________________________________________________________________ +TEST(Result, verifyRunOnNewChunkCallsFinishOnCancellation) { + Result result{[]() -> Result::Generator { + throw ad_utility::CancellationException{ + "verifyRunOnNewChunkCallsFinishOnCancellation"}; + co_return; + }(), + {}}; + uint32_t callCounterGenerator = 0; + uint32_t callCounterFinished = 0; + + result.runOnNewChunkComputed( + [&](const IdTableVocabPair&, std::chrono::microseconds) { + ++callCounterGenerator; + }, + [&](Result::GeneratorState state) { + EXPECT_EQ(state, Result::GeneratorState::CANCELLED); + ++callCounterFinished; + }); + + AD_EXPECT_THROW_WITH_MESSAGE_AND_TYPE( + consumeGenerator(result.idTables()), + HasSubstr("verifyRunOnNewChunkCallsFinishOnCancellation"), + ad_utility::CancellationException); + + EXPECT_EQ(callCounterGenerator, 0); + EXPECT_EQ(callCounterFinished, 1); +} + // _____________________________________________________________________________ TEST(Result, verifyRunOnNewChunkCallsFinishOnPartialConsumption) { uint32_t callCounterGenerator = 0; @@ -262,8 +291,8 @@ TEST(Result, verifyRunOnNewChunkCallsFinishOnPartialConsumption) { [&](const IdTableVocabPair&, std::chrono::microseconds) { ++callCounterGenerator; }, - [&](bool error) { - EXPECT_FALSE(error); + [&](Result::GeneratorState state) { + EXPECT_EQ(state, Result::GeneratorState::FINISHED); ++callCounterFinished; }); From 6a72ee7cb7f49f86b35d0b5bcd63e0c4dad046a8 Mon Sep 17 00:00:00 2001 From: RobinTF <83676088+RobinTF@users.noreply.github.com> Date: Wed, 7 Jan 2026 15:13:19 +0100 Subject: [PATCH 2/3] Try to fix compilation issues --- src/engine/Operation.cpp | 3 ++- src/engine/Result.cpp | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/engine/Operation.cpp b/src/engine/Operation.cpp index 26697306da..5792c9281c 100644 --- a/src/engine/Operation.cpp +++ b/src/engine/Operation.cpp @@ -215,7 +215,8 @@ Result Operation::runComputation(const ad_utility::Timer& timer, return RuntimeInformation::lazilyMaterializedCompleted; case Result::GeneratorState::CANCELLED: return RuntimeInformation::cancelled; - case Result::GeneratorState::FAILED: + default: + AD_CORRECTNESS_CHECK(state == Result::GeneratorState::FAILED); return RuntimeInformation::failed; } }(); diff --git a/src/engine/Result.cpp b/src/engine/Result.cpp index d8abcfeda4..7071da9f3a 100644 --- a/src/engine/Result.cpp +++ b/src/engine/Result.cpp @@ -10,6 +10,7 @@ #include #include "backports/shift.h" +#include "util/CancellationHandle.h" #include "util/Exception.h" #include "util/Generators.h" #include "util/InputRangeUtils.h" From dd003f3b29e7649f33ea3944d33141ae089927e6 Mon Sep 17 00:00:00 2001 From: RobinTF <83676088+RobinTF@users.noreply.github.com> Date: Wed, 7 Jan 2026 15:38:44 +0100 Subject: [PATCH 3/3] Implement sonarcloud suggestion --- src/engine/Operation.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/engine/Operation.cpp b/src/engine/Operation.cpp index 5792c9281c..28859d7b52 100644 --- a/src/engine/Operation.cpp +++ b/src/engine/Operation.cpp @@ -210,13 +210,14 @@ Result Operation::runComputation(const ad_utility::Timer& timer, }, [this](Result::GeneratorState state) { runtimeInfo().status_ = [state]() { + using enum Result::GeneratorState; switch (state) { - case Result::GeneratorState::FINISHED: + case FINISHED: return RuntimeInformation::lazilyMaterializedCompleted; - case Result::GeneratorState::CANCELLED: + case CANCELLED: return RuntimeInformation::cancelled; default: - AD_CORRECTNESS_CHECK(state == Result::GeneratorState::FAILED); + AD_CORRECTNESS_CHECK(state == FAILED); return RuntimeInformation::failed; } }();