Skip to content

Commit

Permalink
Fix the internal variables with COUNT *
Browse files Browse the repository at this point in the history
  • Loading branch information
joka921 committed Jul 26, 2024
1 parent b33b6a8 commit 86c1d1c
Show file tree
Hide file tree
Showing 5 changed files with 54 additions and 39 deletions.
4 changes: 2 additions & 2 deletions src/engine/QueryPlanner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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++)};
}

// _____________________________________________________________________________
Expand Down
10 changes: 8 additions & 2 deletions src/engine/sparqlExpressions/CountStarExpression.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 3 additions & 0 deletions src/global/Constants.h
Original file line number Diff line number Diff line change
Expand Up @@ -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_";
Expand Down
35 changes: 23 additions & 12 deletions test/AggregateExpressionTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<Id>(Eq(Id::makeFromInt(i))));
};

auto totalSize = t.table.size();
using namespace sparqlExpression;
auto m = makeCountStarExpression(false);
EXPECT_THAT(
m->evaluate(&t.context),
::testing::VariantWith<Id>(::testing::Eq(Id::makeFromInt(totalSize))));
EXPECT_THAT(m, matcher(totalSize));

// Add some duplicates.
t.table.push_back(t.table.at(0));
Expand All @@ -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<Id>(::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<Id>(::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
Expand All @@ -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<Id>(::testing::Eq(Id::makeFromInt(totalSize))));
EXPECT_THAT(m, matcher(totalSize));
}

// ______________________________________________________________________________
Expand Down
41 changes: 18 additions & 23 deletions test/QueryPlannerTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -690,17 +690,23 @@ TEST(QueryPlanner, CartesianProductJoin) {
scan("?x", "<b>", "?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};
TransitivePathSide right{std::nullopt, 1, Variable("?y"), 1};
h::expect(
"SELECT ?x ?y WHERE {"
"?x <p>+ ?y }",
h::TransitivePath(
left, right, 1, std::numeric_limits<size_t>::max(),
scan("?_qlever_internal_variable_query_planner_0", "<p>",
"?_qlever_internal_variable_query_planner_1")));
h::TransitivePath(left, right, 1, std::numeric_limits<size_t>::max(),
scan(internalVar(0), "<p>", internalVar(1))));
}

TEST(QueryPlanner, TransitivePathLeftId) {
Expand All @@ -714,10 +720,8 @@ TEST(QueryPlanner, TransitivePathLeftId) {
h::expect(
"SELECT ?y WHERE {"
"<s> <p>+ ?y }",
h::TransitivePath(
left, right, 1, std::numeric_limits<size_t>::max(),
scan("?_qlever_internal_variable_query_planner_0", "<p>",
"?_qlever_internal_variable_query_planner_1")),
h::TransitivePath(left, right, 1, std::numeric_limits<size_t>::max(),
scan(internalVar(0), "<p>", internalVar(1))),
qec);
}

Expand All @@ -732,10 +736,8 @@ TEST(QueryPlanner, TransitivePathRightId) {
h::expect(
"SELECT ?y WHERE {"
"?x <p>+ <o> }",
h::TransitivePath(
left, right, 1, std::numeric_limits<size_t>::max(),
scan("?_qlever_internal_variable_query_planner_0", "<p>",
"?_qlever_internal_variable_query_planner_1")),
h::TransitivePath(left, right, 1, std::numeric_limits<size_t>::max(),
scan(internalVar(0), "<p>", internalVar(1))),
qec);
}

Expand All @@ -747,11 +749,9 @@ TEST(QueryPlanner, TransitivePathBindLeft) {
"SELECT ?x ?y WHERE {"
"<s> <p> ?x."
"?x <p>* ?y }",
h::TransitivePath(
left, right, 0, std::numeric_limits<size_t>::max(),
scan("<s>", "<p>", "?x"),
scan("?_qlever_internal_variable_query_planner_0", "<p>",
"?_qlever_internal_variable_query_planner_1")));
h::TransitivePath(left, right, 0, std::numeric_limits<size_t>::max(),
scan("<s>", "<p>", "?x"),
scan(internalVar(0), "<p>", internalVar(1))));
}

TEST(QueryPlanner, TransitivePathBindRight) {
Expand All @@ -765,9 +765,7 @@ TEST(QueryPlanner, TransitivePathBindRight) {
h::TransitivePath(
left, right, 0, std::numeric_limits<size_t>::max(),
scan("?y", "<p>", "<o>"),
scan("?_qlever_internal_variable_query_planner_0", "<p>",
"?_qlever_internal_variable_query_planner_1",
{Permutation::POS})),
scan(internalVar(0), "<p>", internalVar(1), {Permutation::POS})),
ad_utility::testing::getQec("<x> <p> <o>. <x2> <p> <o2>"));
}

Expand Down Expand Up @@ -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);
};
Expand Down

0 comments on commit 86c1d1c

Please sign in to comment.