code review

ad-freiburg · Oct 25, 2024 · 448934a · 448934a
1 parent 507a896
commit 448934a
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 28 deletions.
diff --git a/src/parser/UpdateClause.h b/src/parser/UpdateClause.h
@@ -64,17 +64,17 @@ struct GraphUpdate {
               std::vector<SparqlTripleSimpleWithGraph> toDelete)
       : toInsert_{std::move(toInsert)}, toDelete_{std::move(toDelete)} {}
 };
+
+// All the available update operations.
+using Operation =
+    std::variant<GraphUpdate, Load, Clear, Drop, Create, Add, Move, Copy>;
 }  // namespace updateClause
 
 namespace parsedQuery {
 struct UpdateClause : ClauseBase {
-  using Operation = std::variant<updateClause::GraphUpdate, updateClause::Load,
-                                 updateClause::Clear, updateClause::Drop,
-                                 updateClause::Create, updateClause::Add,
-                                 updateClause::Move, updateClause::Copy>;
-  Operation op_;
+  updateClause::Operation op_;
 
   UpdateClause() = default;
-  explicit UpdateClause(Operation op) : op_{std::move(op)} {}
+  explicit UpdateClause(updateClause::Operation op) : op_{std::move(op)} {}
 };
 }  // namespace parsedQuery
diff --git a/src/parser/sparqlParser/SparqlQleverVisitor.cpp b/src/parser/sparqlParser/SparqlQleverVisitor.cpp
@@ -449,16 +449,14 @@ ParsedQuery Visitor::visit(Parser::DeleteWhereContext* ctx) {
         AD_CORRECTNESS_CHECK(triple.p_.isVariable() || triple.p_.isIri());
         return SparqlTriple::fromSimple(triple);
       };
-  std::vector<Variable> visibleVariablesSoFar = std::move(visibleVariables_);
-  visibleVariables_.clear();
+  AD_CORRECTNESS_CHECK(visibleVariables_.empty());
   GraphPattern pattern;
   auto triples = visit(ctx->quadPattern());
   pattern._graphPatterns.emplace_back(BasicGraphPattern{
       ad_utility::transform(triples, transformAndRegisterTriple)});
   parsedQuery_._rootGraphPattern = std::move(pattern);
-  auto visibleVariablesWhereClause =
-      std::exchange(visibleVariables_, std::move(visibleVariablesSoFar));
-  parsedQuery_.registerVariablesVisibleInQueryBody(visibleVariablesWhereClause);
+  parsedQuery_.registerVariablesVisibleInQueryBody(visibleVariables_);
+  visibleVariables_.clear();
   // The query body and template are identical. Variables will always be visible
   // - no need to check that.
   parsedQuery_._clause =
@@ -491,13 +489,11 @@ ParsedQuery Visitor::visit(Parser::ModifyContext* ctx) {
           }
         }
       };
-  std::vector<Variable> visibleVariablesSoFar = std::move(visibleVariables_);
-  visibleVariables_.clear();
+  AD_CORRECTNESS_CHECK(visibleVariables_.empty());
   auto graphPattern = visit(ctx->groupGraphPattern());
   parsedQuery_._rootGraphPattern = std::move(graphPattern);
-  auto visibleVariablesWhereClause =
-      std::exchange(visibleVariables_, std::move(visibleVariablesSoFar));
-  parsedQuery_.registerVariablesVisibleInQueryBody(visibleVariablesWhereClause);
+  parsedQuery_.registerVariablesVisibleInQueryBody(visibleVariables_);
+  visibleVariables_.clear();
   auto op = GraphUpdate{};
   visitIf(&op.toInsert_, ctx->insertClause());
   checkTriples(op.toInsert_);
@@ -595,6 +591,8 @@ vector<SparqlTripleSimpleWithGraph> Visitor::transformTriplesTemplate(
 }
 // ____________________________________________________________________________________
 vector<SparqlTripleSimpleWithGraph> Visitor::visit(Parser::QuadsContext* ctx) {
+  // The ordering of the individual triplesTemplate and quadsNotTriples is not
+  // relevant and also not known.
   auto triplesWithGraph = ad_utility::transform(
       ctx->triplesTemplate(), [this](Parser::TriplesTemplateContext* ctx) {
         return transformTriplesTemplate(ctx, std::monostate{});

diff --git a/test/SparqlAntlrParserTestHelpers.h b/test/SparqlAntlrParserTestHelpers.h
@@ -899,37 +899,40 @@ inline auto UpdateQuery =
 inline auto Load = [](bool silent,
                       const ad_utility::triple_component::Iri& source,
                       const std::optional<GraphRef>& target)
-    -> Matcher<const parsedQuery::UpdateClause::Operation&> {
+    -> Matcher<const updateClause::Operation&> {
   return testing::VariantWith<updateClause::Load>(
       testing::AllOf(AD_FIELD(Load, silent_, testing::Eq(silent)),
                      AD_FIELD(Load, source_, testing::Eq(source)),
                      AD_FIELD(Load, target_, testing::Eq(target))));
 };
 
-inline auto Clear = [](bool silent, const GraphRefAll& target)
-    -> Matcher<const parsedQuery::UpdateClause::Operation&> {
+inline auto Clear =
+    [](bool silent,
+       const GraphRefAll& target) -> Matcher<const updateClause::Operation&> {
   return testing::VariantWith<updateClause::Clear>(
       testing::AllOf(AD_FIELD(Clear, silent_, testing::Eq(silent)),
                      AD_FIELD(Clear, target_, testing::Eq(target))));
 };
 
-inline auto Drop = [](bool silent, const GraphRefAll& target)
-    -> Matcher<const parsedQuery::UpdateClause::Operation&> {
+inline auto Drop =
+    [](bool silent,
+       const GraphRefAll& target) -> Matcher<const updateClause::Operation&> {
   return testing::VariantWith<updateClause::Drop>(
       testing::AllOf(AD_FIELD(Drop, silent_, testing::Eq(silent)),
                      AD_FIELD(Drop, target_, testing::Eq(target))));
 };
 
-inline auto Create = [](bool silent, const GraphRef& target)
-    -> Matcher<const parsedQuery::UpdateClause::Operation&> {
+inline auto Create =
+    [](bool silent,
+       const GraphRef& target) -> Matcher<const updateClause::Operation&> {
   return testing::VariantWith<updateClause::Create>(
       testing::AllOf(AD_FIELD(Create, silent_, testing::Eq(silent)),
                      AD_FIELD(Create, target_, testing::Eq(target))));
 };
 
 inline auto Add = [](bool silent, const GraphOrDefault& source,
                      const GraphOrDefault& target)
-    -> Matcher<const parsedQuery::UpdateClause::Operation&> {
+    -> Matcher<const updateClause::Operation&> {
   return testing::VariantWith<updateClause::Add>(
       testing::AllOf(AD_FIELD(Add, silent_, testing::Eq(silent)),
                      AD_FIELD(Add, source_, testing::Eq(source)),
@@ -938,7 +941,7 @@ inline auto Add = [](bool silent, const GraphOrDefault& source,
 
 inline auto Move = [](bool silent, const GraphOrDefault& source,
                       const GraphOrDefault& target)
-    -> Matcher<const parsedQuery::UpdateClause::Operation&> {
+    -> Matcher<const updateClause::Operation&> {
   return testing::VariantWith<updateClause::Move>(
       testing::AllOf(AD_FIELD(Move, silent_, testing::Eq(silent)),
                      AD_FIELD(Move, source_, testing::Eq(source)),
@@ -947,7 +950,7 @@ inline auto Move = [](bool silent, const GraphOrDefault& source,
 
 inline auto Copy = [](bool silent, const GraphOrDefault& source,
                       const GraphOrDefault& target)
-    -> Matcher<const parsedQuery::UpdateClause::Operation&> {
+    -> Matcher<const updateClause::Operation&> {
   return testing::VariantWith<updateClause::Copy>(
       testing::AllOf(AD_FIELD(Copy, silent_, testing::Eq(silent)),
                      AD_FIELD(Copy, source_, testing::Eq(source)),
@@ -958,7 +961,7 @@ inline auto GraphUpdate =
     [](const std::vector<SparqlTripleSimpleWithGraph>& toDelete,
        const std::vector<SparqlTripleSimpleWithGraph>& toInsert,
        const std::optional<ad_utility::triple_component::Iri>& with)
-    -> Matcher<const parsedQuery::UpdateClause::Operation&> {
+    -> Matcher<const updateClause::Operation&> {
   return testing::VariantWith<updateClause::GraphUpdate>(testing::AllOf(
       AD_FIELD(GraphUpdate, toInsert_, testing::ElementsAreArray(toInsert)),
       AD_FIELD(GraphUpdate, toDelete_, testing::ElementsAreArray(toDelete)),
@@ -967,7 +970,7 @@ inline auto GraphUpdate =
 
 // TODO<qup42> add a parameter for the datasets clauses in the graph pattern
 inline auto UpdateClause =
-    [](const Matcher<const parsedQuery::UpdateClause::Operation&>& opMatcher,
+    [](const Matcher<const updateClause::Operation&>& opMatcher,
        const Matcher<const p::GraphPattern&>& graphPatternMatcher)
     -> Matcher<const ::ParsedQuery&> {
   return testing::AllOf(