From 1862b04ac27f7051ce58cf1c0abcb2f3575b499e Mon Sep 17 00:00:00 2001
From: Mislav Bradac <mislav.bradac@memgraph.io>
Date: Fri, 16 Jun 2017 13:40:42 +0200
Subject: [PATCH] Fix bug in StrippedQuery

Reviewers: buda, teon.banek

Reviewed By: buda

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D482
---
 src/query/frontend/ast/ast.hpp                   | 16 ++++++++++++++--
 src/query/frontend/semantic/symbol_generator.cpp |  6 ++++--
 src/query/frontend/stripped.cpp                  |  4 +++-
 src/query/interpreter.hpp                        |  8 ++++----
 tests/unit/query_engine.cpp                      |  9 +++++++++
 5 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/src/query/frontend/ast/ast.hpp b/src/query/frontend/ast/ast.hpp
index d5cc2ef76..01be330d5 100644
--- a/src/query/frontend/ast/ast.hpp
+++ b/src/query/frontend/ast/ast.hpp
@@ -806,7 +806,12 @@ class NodeAtom : public PatternAtom {
   DEFVISITABLE(TreeVisitor<TypedValue>);
   bool Accept(HierarchicalTreeVisitor &visitor) override {
     if (visitor.PreVisit(*this)) {
-      identifier_->Accept(visitor);
+      bool cont = identifier_->Accept(visitor);
+      for (auto &property : properties_) {
+        if (cont) {
+          cont = property.second->Accept(visitor);
+        }
+      }
     }
     return visitor.PostVisit(*this);
   }
@@ -837,7 +842,12 @@ class EdgeAtom : public PatternAtom {
   DEFVISITABLE(TreeVisitor<TypedValue>);
   bool Accept(HierarchicalTreeVisitor &visitor) override {
     if (visitor.PreVisit(*this)) {
-      identifier_->Accept(visitor);
+      bool cont = identifier_->Accept(visitor);
+      for (auto &property : properties_) {
+        if (cont) {
+          cont = property.second->Accept(visitor);
+        }
+      }
     }
     return visitor.PostVisit(*this);
   }
@@ -1396,6 +1406,8 @@ class CachedAst {
     LiteralsPlugger(const Parameters &parameters) : parameters_(parameters) {}
 
     bool Visit(PrimitiveLiteral &literal) override {
+      // TODO: If literal is a part of NamedExpression then we need to change
+      // text in NamedExpression, otherwise wrong header will be returned.
       permanent_assert(
           literal.token_position_ != -1,
           "Use AstPlugLiteralsVisitor only on ast created by parsing queries");
diff --git a/src/query/frontend/semantic/symbol_generator.cpp b/src/query/frontend/semantic/symbol_generator.cpp
index c851ffac1..9bb1e3053 100644
--- a/src/query/frontend/semantic/symbol_generator.cpp
+++ b/src/query/frontend/semantic/symbol_generator.cpp
@@ -281,7 +281,8 @@ bool SymbolGenerator::PreVisit(NodeAtom &node_atom) {
     kv.second->Accept(*this);
   }
   scope_.in_property_map = false;
-  return true;
+  node_atom.identifier_->Accept(*this);
+  return false;
 }
 
 bool SymbolGenerator::PostVisit(NodeAtom &node_atom) {
@@ -310,7 +311,8 @@ bool SymbolGenerator::PreVisit(EdgeAtom &edge_atom) {
     kv.second->Accept(*this);
   }
   scope_.in_property_map = false;
-  return true;
+  edge_atom.identifier_->Accept(*this);
+  return false;
 }
 
 bool SymbolGenerator::PostVisit(EdgeAtom &edge_atom) {
diff --git a/src/query/frontend/stripped.cpp b/src/query/frontend/stripped.cpp
index ef4098792..49ea675ee 100644
--- a/src/query/frontend/stripped.cpp
+++ b/src/query/frontend/stripped.cpp
@@ -42,7 +42,9 @@ StrippedQuery::StrippedQuery(const std::string &query) {
 
   // Convert tokens to strings, perform lowercasing and filtering.
   for (const auto *token : tokens) {
-    int position = token->getTokenIndex();
+    // Position is calculated in query after stripping and whitespace
+    // normalisation, not before.
+    int position = token_strings.size() * 2;
 
     switch (token->getType()) {
       case CypherLexer::UNION:
diff --git a/src/query/interpreter.hpp b/src/query/interpreter.hpp
index 0d4cbdb80..8b1688476 100644
--- a/src/query/interpreter.hpp
+++ b/src/query/interpreter.hpp
@@ -32,9 +32,6 @@ class Interpreter : public Loggable {
     Context ctx(config, db_accessor);
     std::map<std::string, TypedValue> summary;
 
-    // query -> stripped query
-    StrippedQuery stripped(query);
-
     // stripped query -> high level tree
     AstTreeStorage ast_storage = [&]() {
       if (!FLAGS_ast_cache) {
@@ -48,8 +45,11 @@ class Interpreter : public Loggable {
         return std::move(visitor.storage());
       }
 
+      // query -> stripped query
+      StrippedQuery stripped(query);
+
       auto ast_cache_accessor = ast_cache_.access();
-      auto it = ast_cache_accessor.find(query::StrippedQuery(query).hash());
+      auto it = ast_cache_accessor.find(stripped.hash());
       if (it == ast_cache_accessor.end()) {
         // stripped query -> AST
         frontend::opencypher::Parser parser(stripped.query());
diff --git a/tests/unit/query_engine.cpp b/tests/unit/query_engine.cpp
index c365da565..b6372fb94 100644
--- a/tests/unit/query_engine.cpp
+++ b/tests/unit/query_engine.cpp
@@ -59,5 +59,14 @@ TEST(QueryEngine, AstCache) {
     ASSERT_EQ(stream.GetResults()[0].size(), 1U);
     ASSERT_EQ(stream.GetResults()[0][0].Value<double>(), 11.5);
   }
+  {
+    // Cached ast, same literals, different whitespaces.
+    ResultStreamFaker stream;
+    auto dba = dbms.active();
+    engine.Run("RETURN 10.5+1", *dba, stream);
+    ASSERT_EQ(stream.GetResults().size(), 1U);
+    ASSERT_EQ(stream.GetResults()[0].size(), 1U);
+    ASSERT_EQ(stream.GetResults()[0][0].Value<double>(), 11.5);
+  }
 }
 }