diff --git a/src/memgraph.cpp b/src/memgraph.cpp index f8342edc4..1973047e6 100644 --- a/src/memgraph.cpp +++ b/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{ diff --git a/src/memgraph_ha.cpp b/src/memgraph_ha.cpp index 5f1ff0673..4bafe3b6c 100644 --- a/src/memgraph_ha.cpp +++ b/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; diff --git a/src/memgraph_init.hpp b/src/memgraph_init.hpp index 0fd5880ea..c896720e1 100644 --- a/src/memgraph_init.hpp +++ b/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; }; diff --git a/src/query/interpreter.cpp b/src/query/interpreter.cpp index 278e19cb6..b88dcfa27 100644 --- a/src/query/interpreter.cpp +++ b/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 ¶meters, DbAccessor *db_accessor) { auto plan_cache_access = interpreter_context_->plan_cache.access(); diff --git a/src/query/interpreter.hpp b/src/query/interpreter.hpp index b68e18b1f..7a5868c78 100644 --- a/src/query/interpreter.hpp +++ b/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. diff --git a/tests/benchmark/expansion.cpp b/tests/benchmark/expansion.cpp index 3f07b1145..0dfc4425a 100644 --- a/tests/benchmark/expansion.cpp +++ b/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 { diff --git a/tests/feature_benchmark/kafka/benchmark.cpp b/tests/feature_benchmark/kafka/benchmark.cpp index 17e84f7d0..cd8493c50 100644 --- a/tests/feature_benchmark/kafka/benchmark.cpp +++ b/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}; diff --git a/tests/manual/repl.cpp b/tests/manual/repl.cpp index 4a53a9885..4dccdd935 100644 --- a/tests/manual/repl.cpp +++ b/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; diff --git a/tests/manual/single_query.cpp b/tests/manual/single_query.cpp index a5c11c974..6a964e86c 100644 --- a/tests/manual/single_query.cpp +++ b/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()); diff --git a/tests/unit/database_dump.cpp b/tests/unit/database_dump.cpp index 9fa5e982e..ada7269e7 100644 --- a/tests/unit/database_dump.cpp +++ b/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()); diff --git a/tests/unit/database_transaction_timeout.cpp b/tests/unit/database_transaction_timeout.cpp index 4183e5e28..8d095f404 100644 --- a/tests/unit/database_transaction_timeout.cpp +++ b/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); diff --git a/tests/unit/interpreter.cpp b/tests/unit/interpreter.cpp index 939d41a71..b47105b58 100644 --- a/tests/unit/interpreter.cpp +++ b/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, diff --git a/tests/unit/query_plan_edge_cases.cpp b/tests/unit/query_plan_edge_cases.cpp index b0b2dc49f..ddc3eed93 100644 --- a/tests/unit/query_plan_edge_cases.cpp +++ b/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());