diff --git a/src/engine/QueryPlanner.cpp b/src/engine/QueryPlanner.cpp index a1978b3bd1..7f87cd7de8 100644 --- a/src/engine/QueryPlanner.cpp +++ b/src/engine/QueryPlanner.cpp @@ -875,8 +875,8 @@ ParsedQuery::GraphPattern QueryPlanner::uniteGraphPatterns( // _____________________________________________________________________________ Variable QueryPlanner::generateUniqueVarName() { - return Variable{"?_qlever_internal_variable_query_planner_" + - std::to_string(_internalVarCount++)}; + return Variable{absl::StrCat(INTERNAL_VARIABLE_QUERY_PLANNER_PREFIX, + _internalVarCount++)}; } // _____________________________________________________________________________ diff --git a/src/engine/sparqlExpressions/CountStarExpression.cpp b/src/engine/sparqlExpressions/CountStarExpression.cpp index bb3ef58e8a..416f653c20 100644 --- a/src/engine/sparqlExpressions/CountStarExpression.cpp +++ b/src/engine/sparqlExpressions/CountStarExpression.cpp @@ -36,11 +36,17 @@ class CountStarExpression : public SparqlExpression { // We only need to copy columns that are actually visible. Columns that are // hidden, e.g. because they weren't selectedd in a subquery shouldn't be // part of the DISTINCT computation. - table.setNumColumns(ctx->_variableToColumnMap.size()); + + auto varToColNoInternalVariables = + ctx->_variableToColumnMap | + std::views::filter([](const auto& varAndIdx) { + return !varAndIdx.first.name().starts_with(INTERNAL_VARIABLE_PREFIX); + }); + table.setNumColumns(std::ranges::distance(varToColNoInternalVariables)); table.resize(ctx->size()); size_t targetIdx = 0; for (const auto& [sourceIdx, _] : - ctx->_variableToColumnMap | std::views::values) { + varToColNoInternalVariables | std::views::values) { const auto& sourceColumn = ctx->_inputTable.getColumn(sourceIdx); std::ranges::copy(sourceColumn.begin() + ctx->_beginIndex, sourceColumn.begin() + ctx->_endIndex, diff --git a/src/global/Constants.h b/src/global/Constants.h index b8650baeb8..784123c57e 100644 --- a/src/global/Constants.h +++ b/src/global/Constants.h @@ -69,6 +69,9 @@ static const std::string INTERNAL_VARIABLE_PREFIX = constexpr std::string_view INTERNAL_BLANKNODE_VARIABLE_PREFIX = "?_QLever_internal_variable_bn_"; +constexpr std::string_view INTERNAL_VARIABLE_QUERY_PLANNER_PREFIX = + "?_QLever_internal_variable_qp_"; + static constexpr std::string_view SCORE_VARIABLE_PREFIX = "?ql_score_"; static constexpr std::string_view MATCHINGWORD_VARIABLE_PREFIX = "?ql_matchingword_"; diff --git a/test/AggregateExpressionTest.cpp b/test/AggregateExpressionTest.cpp index 3194be6d8e..329d7086ff 100644 --- a/test/AggregateExpressionTest.cpp +++ b/test/AggregateExpressionTest.cpp @@ -105,12 +105,22 @@ TEST(AggregateExpression, count) { // ______________________________________________________________________________ TEST(AggregateExpression, CountStar) { auto t = TestContext{}; + // A matcher that first clears the cache and then checks that the result of + // evaluating a `SparqlExpression::Ptr` yields the single integer ID that + // stores `i`. + auto matcher = [&](int64_t i) { + auto evaluate = [&](const SparqlExpression::Ptr& expr) { + return expr->evaluate(&t.context); + }; + using namespace ::testing; + t.qec->getQueryTreeCache().clearAll(); + return ResultOf(evaluate, VariantWith(Eq(Id::makeFromInt(i)))); + }; + auto totalSize = t.table.size(); using namespace sparqlExpression; auto m = makeCountStarExpression(false); - EXPECT_THAT( - m->evaluate(&t.context), - ::testing::VariantWith(::testing::Eq(Id::makeFromInt(totalSize)))); + EXPECT_THAT(m, matcher(totalSize)); // Add some duplicates. t.table.push_back(t.table.at(0)); @@ -120,16 +130,12 @@ TEST(AggregateExpression, CountStar) { t.context._endIndex += 3; - t.qec->getQueryTreeCache().clearAll(); // A COUNT * has now a size which is larger by 3, but a COUNT DISTINCT * still // has the same size. - EXPECT_THAT(m->evaluate(&t.context), ::testing::VariantWith(::testing::Eq( - Id::makeFromInt(totalSize + 3)))); - + EXPECT_THAT(m, matcher(totalSize + 3)); // We have added two duplicates, and one new distinct row m = makeCountStarExpression(true); - EXPECT_THAT(m->evaluate(&t.context), ::testing::VariantWith(::testing::Eq( - Id::makeFromInt(totalSize + 1)))); + EXPECT_THAT(m, matcher(totalSize + 1)); // If we modify the `varToColMap` such that it doesn't contain our unique // value in column 0, then the number of distinct entries goes back to where @@ -139,10 +145,15 @@ TEST(AggregateExpression, CountStar) { t.varToColMap[Variable{"?x"}] = { 1, ColumnIndexAndTypeInfo::UndefStatus::AlwaysDefined}; + EXPECT_THAT(m, matcher(totalSize)); + + // This variable is internal, so it doesn't count towards the `COUNT(DISTINCT + // *)` and doesn't change the result. + t.varToColMap[Variable{ + absl::StrCat(INTERNAL_VARIABLE_PREFIX, "someInternalVar")}] = { + 0, ColumnIndexAndTypeInfo::UndefStatus::AlwaysDefined}; t.qec->getQueryTreeCache().clearAll(); - EXPECT_THAT( - m->evaluate(&t.context), - ::testing::VariantWith(::testing::Eq(Id::makeFromInt(totalSize)))); + EXPECT_THAT(m, matcher(totalSize)); } // ______________________________________________________________________________ diff --git a/test/QueryPlannerTest.cpp b/test/QueryPlannerTest.cpp index 7451016f2a..42f98286f2 100644 --- a/test/QueryPlannerTest.cpp +++ b/test/QueryPlannerTest.cpp @@ -690,6 +690,14 @@ TEST(QueryPlanner, CartesianProductJoin) { scan("?x", "", "?c"))); } +namespace { +// A helper function to recreate the internal variables added by the query +// planner for transitive paths. +std::string internalVar(int i) { + return absl::StrCat(INTERNAL_VARIABLE_QUERY_PLANNER_PREFIX, i); +} +} // namespace + TEST(QueryPlanner, TransitivePathUnbound) { auto scan = h::IndexScanFromStrings; TransitivePathSide left{std::nullopt, 0, Variable("?x"), 0}; @@ -697,10 +705,8 @@ TEST(QueryPlanner, TransitivePathUnbound) { h::expect( "SELECT ?x ?y WHERE {" "?x

+ ?y }", - h::TransitivePath( - left, right, 1, std::numeric_limits::max(), - scan("?_qlever_internal_variable_query_planner_0", "

", - "?_qlever_internal_variable_query_planner_1"))); + h::TransitivePath(left, right, 1, std::numeric_limits::max(), + scan(internalVar(0), "

", internalVar(1)))); } TEST(QueryPlanner, TransitivePathLeftId) { @@ -714,10 +720,8 @@ TEST(QueryPlanner, TransitivePathLeftId) { h::expect( "SELECT ?y WHERE {" "

+ ?y }", - h::TransitivePath( - left, right, 1, std::numeric_limits::max(), - scan("?_qlever_internal_variable_query_planner_0", "

", - "?_qlever_internal_variable_query_planner_1")), + h::TransitivePath(left, right, 1, std::numeric_limits::max(), + scan(internalVar(0), "

", internalVar(1))), qec); } @@ -732,10 +736,8 @@ TEST(QueryPlanner, TransitivePathRightId) { h::expect( "SELECT ?y WHERE {" "?x

+ }", - h::TransitivePath( - left, right, 1, std::numeric_limits::max(), - scan("?_qlever_internal_variable_query_planner_0", "

", - "?_qlever_internal_variable_query_planner_1")), + h::TransitivePath(left, right, 1, std::numeric_limits::max(), + scan(internalVar(0), "

", internalVar(1))), qec); } @@ -747,11 +749,9 @@ TEST(QueryPlanner, TransitivePathBindLeft) { "SELECT ?x ?y WHERE {" "

?x." "?x

* ?y }", - h::TransitivePath( - left, right, 0, std::numeric_limits::max(), - scan("", "

", "?x"), - scan("?_qlever_internal_variable_query_planner_0", "

", - "?_qlever_internal_variable_query_planner_1"))); + h::TransitivePath(left, right, 0, std::numeric_limits::max(), + scan("", "

", "?x"), + scan(internalVar(0), "

", internalVar(1)))); } TEST(QueryPlanner, TransitivePathBindRight) { @@ -765,9 +765,7 @@ TEST(QueryPlanner, TransitivePathBindRight) { h::TransitivePath( left, right, 0, std::numeric_limits::max(), scan("?y", "

", ""), - scan("?_qlever_internal_variable_query_planner_0", "

", - "?_qlever_internal_variable_query_planner_1", - {Permutation::POS})), + scan(internalVar(0), "

", internalVar(1), {Permutation::POS})), ad_utility::testing::getQec("

.

")); } @@ -965,9 +963,6 @@ TEST(QueryPlanner, TextLimit) { } TEST(QueryPlanner, NonDistinctVariablesInTriple) { - auto internalVar = [](int i) { - return absl::StrCat("?_qlever_internal_variable_query_planner_", i); - }; auto eq = [](std::string_view l, std::string_view r) { return absl::StrCat(l, "=", r); };