Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use one LocalVocab for each block during lazy evaluation #1567

Merged
merged 10 commits into from
Oct 22, 2024
7 changes: 3 additions & 4 deletions src/engine/Bind.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -131,13 +131,12 @@
LOG(DEBUG) << "BIND result computation done." << std::endl;
return {std::move(result), resultSortedOn(), std::move(localVocab)};
}
auto localVocab = std::make_shared<LocalVocab>();
auto generator =
[](auto applyBind,
std::shared_ptr<const Result> result) -> Result::Generator {
for (Result::IdTableVocabPair& pair : result->idTables()) {
IdTable idTable = applyBind(std::move(pair.idTable_), &pair.localVocab_);
co_yield {std::move(idTable), std::move(pair.localVocab_)};
for (auto& [idTable, localVocab] : result->idTables()) {
IdTable resultTable = applyBind(std::move(idTable), &localVocab);
co_yield {std::move(resultTable), std::move(localVocab)};
}
}(std::move(applyBind), std::move(subRes));
return {std::move(generator), resultSortedOn()};
Expand Down Expand Up @@ -196,7 +195,7 @@
for (auto& resultValue : resultGenerator) {
outputColumn[i] =
sparqlExpression::detail::constantExpressionResultToId(
std::move(resultValue), *localVocab);

Check warning on line 198 in src/engine/Bind.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/Bind.cpp#L198

Added line #L198 was not covered by tests
i++;
checkCancellation();
}
Expand Down
8 changes: 4 additions & 4 deletions src/engine/Filter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ ProtoResult Filter::computeResult(bool requestLaziness) {

if (requestLaziness) {
return {[](auto subRes, auto* self) -> Result::Generator {
for (Result::IdTableVocabPair& pair : subRes->idTables()) {
IdTable result = self->filterIdTable(
subRes->sortedBy(), pair.idTable_, pair.localVocab_);
co_yield {std::move(result), std::move(pair.localVocab_)};
for (auto& [idTable, localVocab] : subRes->idTables()) {
IdTable result = self->filterIdTable(subRes->sortedBy(),
idTable, localVocab);
co_yield {std::move(result), std::move(localVocab)};
}
}(std::move(subRes), this),
resultSortedOn()};
Expand Down
3 changes: 2 additions & 1 deletion src/engine/GroupBy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,8 @@ Result::Generator GroupBy::computeResultLazily(
// Reuse buffer if not moved out
resultTable = std::move(outputPair.idTable_);
resultTable.clear();
currentLocalVocab = LocalVocab{};
// Keep last local vocab for next commit.
currentLocalVocab = std::move(storedLocalVocabs.back());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessarily correct.
If your group is very long (e.g. from 5 IdTables ago) , then you also have to store the corresponding local vocab from exactly that group.
So you have to probably have alocal vocab that is associated with the currentGroupBlock and updated exactly when you do the commitRow call (because then you implicitly updated the next group block ,isn't that true?

storedLocalVocabs.clear();
RobinTF marked this conversation as resolved.
Show resolved Hide resolved
}
}
Expand Down
14 changes: 6 additions & 8 deletions src/engine/Union.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -264,10 +264,9 @@ Result::Generator Union::computeResultLazily(
transformToCorrectColumnFormat(result1->idTable().clone(), permutation),
result1->getCopyOfLocalVocab()};
} else {
for (Result::IdTableVocabPair& pair : result1->idTables()) {
co_yield {
transformToCorrectColumnFormat(std::move(pair.idTable_), permutation),
std::move(pair.localVocab_)};
for (auto& [idTable, localVocab] : result1->idTables()) {
co_yield {transformToCorrectColumnFormat(std::move(idTable), permutation),
std::move(localVocab)};
}
}
permutation = computePermutation<false>();
Expand All @@ -276,10 +275,9 @@ Result::Generator Union::computeResultLazily(
transformToCorrectColumnFormat(result2->idTable().clone(), permutation),
result2->getCopyOfLocalVocab()};
} else {
for (Result::IdTableVocabPair& pair : result2->idTables()) {
co_yield {
transformToCorrectColumnFormat(std::move(pair.idTable_), permutation),
std::move(pair.localVocab_)};
for (auto& [idTable, localVocab] : result2->idTables()) {
co_yield {transformToCorrectColumnFormat(std::move(idTable), permutation),
std::move(localVocab)};
}
}
}
4 changes: 4 additions & 0 deletions test/FilterTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ ValueId asBool(bool value) { return Id::makeFromBool(value); }
std::vector<IdTable> toVector(Result::Generator generator) {
std::vector<IdTable> result;
for (auto& pair : generator) {
// IMPORTANT: The `LocalVocab` contained in the pair will be destroyed at
// the end of the iteration. The underlying assumption is that the
// `LocalVocab` will be empty and the `IdTable` won't contain any dangling
// references.
result.push_back(std::move(pair.idTable_));
RobinTF marked this conversation as resolved.
Show resolved Hide resolved
}
return result;
Expand Down
Loading