Pull out InterpreterContext

Summary: Make `InterpreterContext` a top level instead of a nested struct

Reviewers: teon.banek

Reviewed By: teon.banek

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D2512

src/memgraph.cpp

@@ -97,7 +97,7 @@ void SingleNodeMain() {
 #else
   database::GraphDb db;
 #endif
-  query::Interpreter::InterpreterContext interpreter_context;
+  query::InterpreterContext interpreter_context;
   SessionData session_data{&db, &interpreter_context, &auth, &audit_log};
 
   integrations::kafka::Streams kafka_streams{

src/memgraph_ha.cpp

@@ -40,7 +40,7 @@ void SingleNodeHAMain() {
   auto durability_directory = std::filesystem::path(FLAGS_durability_directory);
 
   database::GraphDb db;
-  query::Interpreter::InterpreterContext interpreter_context;
+  query::InterpreterContext interpreter_context;
   SessionData session_data{&db, &interpreter_context, nullptr, nullptr};
 
   ServerContext context;

src/memgraph_init.hpp

@@ -32,14 +32,14 @@ struct SessionData {
   // Explicit constructor here to ensure that pointers to all objects are
   // supplied.
   SessionData(database::GraphDb *_db,
-              query::Interpreter::InterpreterContext *_interpreter_context,
+              query::InterpreterContext *_interpreter_context,
               auth::Auth *_auth, audit::Log *_audit_log)
       : db(_db),
         interpreter_context(_interpreter_context),
         auth(_auth),
         audit_log(_audit_log) {}
   database::GraphDb *db;
-  query::Interpreter::InterpreterContext *interpreter_context;
+  query::InterpreterContext *interpreter_context;
   auth::Auth *auth;
   audit::Log *audit_log;
 };

src/query/interpreter.cpp

@@ -93,7 +93,7 @@ class SingleNodeLogicalPlan final : public LogicalPlan {
   SymbolTable symbol_table_;
 };
 
-Interpreter::CachedPlan::CachedPlan(std::unique_ptr<LogicalPlan> plan)
+CachedPlan::CachedPlan(std::unique_ptr<LogicalPlan> plan)
     : plan_(std::move(plan)) {}
 
 void Interpreter::PrettyPrintPlan(const DbAccessor &dba,
@@ -1154,7 +1154,7 @@ Interpreter::Results Interpreter::operator()(
                  /* is_profile_query */ false, callback.should_abort_query);
 }
 
-std::shared_ptr<Interpreter::CachedPlan> Interpreter::CypherQueryToPlan(
+std::shared_ptr<CachedPlan> Interpreter::CypherQueryToPlan(
     HashType query_hash, CypherQuery *query, AstStorage ast_storage,
     const Parameters &parameters, DbAccessor *db_accessor) {
   auto plan_cache_access = interpreter_context_->plan_cache.access();

src/query/interpreter.hpp

@@ -39,83 +39,89 @@ class LogicalPlan {
   virtual const AstStorage &GetAstStorage() const = 0;
 };
 
-class Interpreter {
- private:
-  class CachedPlan {
-   public:
-    CachedPlan(std::unique_ptr<LogicalPlan> plan);
-
-    const auto &plan() const { return plan_->GetRoot(); }
-    double cost() const { return plan_->GetCost(); }
-    const auto &symbol_table() const { return plan_->GetSymbolTable(); }
-    const auto &ast_storage() const { return plan_->GetAstStorage(); }
-
-    bool IsExpired() const {
-      return cache_timer_.Elapsed() >
-             std::chrono::seconds(FLAGS_query_plan_cache_ttl);
-    };
-
-   private:
-    std::unique_ptr<LogicalPlan> plan_;
-    utils::Timer cache_timer_;
-  };
-
-  struct CachedQuery {
-    AstStorage ast_storage;
-    Query *query;
-    std::vector<AuthQuery::Privilege> required_privileges;
-  };
-
-  struct QueryCacheEntry {
-    bool operator==(const QueryCacheEntry &other) const {
-      return first == other.first;
-    }
-    bool operator<(const QueryCacheEntry &other) const {
-      return first < other.first;
-    }
-    bool operator==(const HashType &other) const { return first == other; }
-    bool operator<(const HashType &other) const { return first < other; }
-
-    HashType first;
-    // TODO: Maybe store the query string here and use it as a key with the hash
-    // so that we eliminate the risk of hash collisions.
-    CachedQuery second;
-  };
-
-  struct PlanCacheEntry {
-    bool operator==(const PlanCacheEntry &other) const {
-      return first == other.first;
-    }
-    bool operator<(const PlanCacheEntry &other) const {
-      return first < other.first;
-    }
-    bool operator==(const HashType &other) const { return first == other; }
-    bool operator<(const HashType &other) const { return first < other; }
-
-    HashType first;
-    // TODO: Maybe store the query string here and use it as a key with the hash
-    // so that we eliminate the risk of hash collisions.
-    std::shared_ptr<CachedPlan> second;
-  };
-
+class CachedPlan {
  public:
-  struct InterpreterContext {
-    // Antlr has singleton instance that is shared between threads. It is
-    // protected by locks inside of antlr. Unfortunately, they are not protected
-    // in a very good way. Once we have antlr version without race conditions we
-    // can remove this lock. This will probably never happen since antlr
-    // developers introduce more bugs in each version. Fortunately, we have
-    // cache so this lock probably won't impact performance much...
-    utils::SpinLock antlr_lock;
-    bool is_tsc_available{utils::CheckAvailableTSC()};
+  explicit CachedPlan(std::unique_ptr<LogicalPlan> plan);
 
-    auth::Auth *auth{nullptr};
-    integrations::kafka::Streams *kafka_streams{nullptr};
+  const auto &plan() const { return plan_->GetRoot(); }
+  double cost() const { return plan_->GetCost(); }
+  const auto &symbol_table() const { return plan_->GetSymbolTable(); }
+  const auto &ast_storage() const { return plan_->GetAstStorage(); }
 
-    utils::SkipList<QueryCacheEntry> ast_cache;
-    utils::SkipList<PlanCacheEntry> plan_cache;
+  bool IsExpired() const {
+    return cache_timer_.Elapsed() >
+           std::chrono::seconds(FLAGS_query_plan_cache_ttl);
   };
 
+ private:
+  std::unique_ptr<LogicalPlan> plan_;
+  utils::Timer cache_timer_;
+};
+
+struct CachedQuery {
+  AstStorage ast_storage;
+  Query *query;
+  std::vector<AuthQuery::Privilege> required_privileges;
+};
+
+struct QueryCacheEntry {
+  bool operator==(const QueryCacheEntry &other) const {
+    return first == other.first;
+  }
+  bool operator<(const QueryCacheEntry &other) const {
+    return first < other.first;
+  }
+  bool operator==(const HashType &other) const { return first == other; }
+  bool operator<(const HashType &other) const { return first < other; }
+
+  HashType first;
+  // TODO: Maybe store the query string here and use it as a key with the hash
+  // so that we eliminate the risk of hash collisions.
+  CachedQuery second;
+};
+
+struct PlanCacheEntry {
+  bool operator==(const PlanCacheEntry &other) const {
+    return first == other.first;
+  }
+  bool operator<(const PlanCacheEntry &other) const {
+    return first < other.first;
+  }
+  bool operator==(const HashType &other) const { return first == other; }
+  bool operator<(const HashType &other) const { return first < other; }
+
+  HashType first;
+  // TODO: Maybe store the query string here and use it as a key with the hash
+  // so that we eliminate the risk of hash collisions.
+  std::shared_ptr<CachedPlan> second;
+};
+
+/**
+ * Holds data shared between multiple `Interpreter` instances (which might be
+ * running concurrently).
+ *
+ * Users should initialize the context but should not modify it after it has
+ * been passed to an `Interpreter` instance.
+ */
+struct InterpreterContext {
+  // Antlr has singleton instance that is shared between threads. It is
+  // protected by locks inside of antlr. Unfortunately, they are not protected
+  // in a very good way. Once we have antlr version without race conditions we
+  // can remove this lock. This will probably never happen since antlr
+  // developers introduce more bugs in each version. Fortunately, we have
+  // cache so this lock probably won't impact performance much...
+  utils::SpinLock antlr_lock;
+  bool is_tsc_available{utils::CheckAvailableTSC()};
+
+  auth::Auth *auth{nullptr};
+  integrations::kafka::Streams *kafka_streams{nullptr};
+
+  utils::SkipList<QueryCacheEntry> ast_cache;
+  utils::SkipList<PlanCacheEntry> plan_cache;
+};
+
+class Interpreter {
+ public:
   /**
    * Wraps a `Query` that was created as a result of parsing a query string
    * along with its privileges.

tests/benchmark/expansion.cpp

@@ -13,7 +13,7 @@ class ExpansionBenchFixture : public benchmark::Fixture {
   // GraphDb shouldn't be global constructed/destructed. See
   // documentation in database/single_node/graph_db.hpp for details.
   std::optional<database::GraphDb> db_;
-  query::Interpreter::InterpreterContext interpreter_context_;
+  query::InterpreterContext interpreter_context_;
   query::Interpreter interpreter_{&interpreter_context_};
 
   void SetUp(const benchmark::State &state) override {

tests/feature_benchmark/kafka/benchmark.cpp

@@ -38,7 +38,7 @@ void KafkaBenchmarkMain() {
                        audit::kBufferSizeDefault,
                        audit::kBufferFlushIntervalMillisDefault};
 
-  query::Interpreter::InterpreterContext interpreter_context;
+  query::InterpreterContext interpreter_context;
   database::GraphDb db;
   SessionData session_data{&db, &interpreter_context, &auth, &audit_log};
 

tests/manual/repl.cpp

@@ -297,7 +297,7 @@ int main(int argc, char *argv[]) {
   std::cout << "Generating graph..." << std::endl;
   //  fill_db;
   random_generate(db, node_count, edge_count);
-  query::Interpreter::InterpreterContext interpreter_context;
+  query::InterpreterContext interpreter_context;
   query::Interpreter interpreter{&interpreter_context};
   query::Repl(&db, &interpreter);
   return 0;

tests/manual/single_query.cpp

@@ -15,7 +15,7 @@ int main(int argc, char *argv[]) {
   auto dba = db.Access();
   query::DbAccessor query_dba(&dba);
   ResultStreamFaker<query::TypedValue> stream;
-  query::Interpreter::InterpreterContext interpreter_context;
+  query::InterpreterContext interpreter_context;
   auto results = query::Interpreter(&interpreter_context)(
       argv[1], &query_dba, {}, false, utils::NewDeleteResource());
   stream.Header(results.header());

tests/unit/database_dump.cpp

@@ -185,7 +185,7 @@ void Execute(GraphDbAccessor *dba, const std::string &query) {
   CHECK(dba);
   ResultStreamFaker<query::TypedValue> results;
   query::DbAccessor query_dba(dba);
-  query::Interpreter::InterpreterContext interpreter_context;
+  query::InterpreterContext interpreter_context;
   query::Interpreter (&interpreter_context)(query, &query_dba, {}, false,
                                             utils::NewDeleteResource())
       .PullAll(results);
@@ -583,7 +583,7 @@ TEST(DumpTest, ExecuteDumpDatabase) {
     query::DbAccessor query_dba(&dba);
     const std::string query = "DUMP DATABASE";
     ResultStreamFaker<query::TypedValue> stream;
-    query::Interpreter::InterpreterContext interpreter_context;
+    query::InterpreterContext interpreter_context;
     auto results = query::Interpreter(&interpreter_context)(
         query, &query_dba, {}, false, utils::NewDeleteResource());
     stream.Header(results.header());

tests/unit/database_transaction_timeout.cpp

@@ -10,7 +10,7 @@ DECLARE_int32(query_execution_time_sec);
 TEST(TransactionTimeout, TransactionTimeout) {
   FLAGS_query_execution_time_sec = 3;
   database::GraphDb db;
-  query::Interpreter::InterpreterContext interpreter_context;
+  query::InterpreterContext interpreter_context;
   query::Interpreter interpreter(&interpreter_context);
   auto interpret = [&](auto &dba, const std::string &query) {
     query::DbAccessor query_dba(&dba);

tests/unit/interpreter.cpp

@@ -15,7 +15,7 @@
 class InterpreterTest : public ::testing::Test {
  protected:
   database::GraphDb db_;
-  query::Interpreter::InterpreterContext interpreter_context_;
+  query::InterpreterContext interpreter_context_;
   query::Interpreter interpreter_{&interpreter_context_};
 
   auto Interpret(const std::string &query,

tests/unit/query_plan_edge_cases.cpp

@@ -41,7 +41,7 @@ class QueryExecution : public testing::Test {
   auto Execute(const std::string &query) {
     query::DbAccessor query_dba(&*dba_);
     ResultStreamFaker<query::TypedValue> stream;
-    query::Interpreter::InterpreterContext interpreter_context;
+    query::InterpreterContext interpreter_context;
     auto results = query::Interpreter(&interpreter_context)(
         query, &query_dba, {}, false, utils::NewDeleteResource());
     stream.Header(results.header());