From 7a9c4f5ec4e8f68746ec59aa7265df3b82539894 Mon Sep 17 00:00:00 2001 From: gvolfing <107616712+gvolfing@users.noreply.github.com> Date: Wed, 6 Dec 2023 22:52:28 +0100 Subject: [PATCH] Fix logic in RelWithDebInfo mode (#1397) In and only in RelWithDebInfo mode the access of the maybe_unused variable results in segfaults, this change is making sure that does no happen ever if the maybe_unused variable is nullopt without changing the overall logic. --- .../frontend/ast/cypher_main_visitor.cpp | 71 +++++++++++++------ 1 file changed, 51 insertions(+), 20 deletions(-) diff --git a/src/query/frontend/ast/cypher_main_visitor.cpp b/src/query/frontend/ast/cypher_main_visitor.cpp index 16a5f4828..7002ee4b9 100644 --- a/src/query/frontend/ast/cypher_main_visitor.cpp +++ b/src/query/frontend/ast/cypher_main_visitor.cpp @@ -1276,28 +1276,59 @@ antlrcpp::Any CypherMainVisitor::visitCallProcedure(MemgraphCypher::CallProcedur call_proc->result_identifiers_.push_back(storage_->Create(result_alias)); } } else { - const auto &maybe_found = - procedure::FindProcedure(procedure::gModuleRegistry, call_proc->procedure_name_, utils::NewDeleteResource()); - if (!maybe_found) { - throw SemanticException("There is no procedure named '{}'.", call_proc->procedure_name_); + call_proc->is_write_ = maybe_found->second->info.is_write; + + auto *yield_ctx = ctx->yieldProcedureResults(); + if (!yield_ctx) { + if (!maybe_found->second->results.empty() && !call_proc->void_procedure_) { + throw SemanticException( + "CALL without YIELD may only be used on procedures which do not " + "return any result fields."); + } + // When we return, we will release the lock on modules. This means that + // someone may reload the procedure and change the result signature. But to + // keep the implementation simple, we ignore the case as the rest of the + // code doesn't really care whether we yield or not, so it should not break. + return call_proc; } - const auto &[module, proc] = *maybe_found; - call_proc->result_fields_.reserve(proc->results.size()); - call_proc->result_identifiers_.reserve(proc->results.size()); - for (const auto &[result_name, desc] : proc->results) { - bool is_deprecated = desc.second; - if (is_deprecated) continue; - call_proc->result_fields_.emplace_back(result_name); - call_proc->result_identifiers_.push_back(storage_->Create(std::string(result_name))); + if (yield_ctx->getTokens(MemgraphCypher::ASTERISK).empty()) { + call_proc->result_fields_.reserve(yield_ctx->procedureResult().size()); + call_proc->result_identifiers_.reserve(yield_ctx->procedureResult().size()); + for (auto *result : yield_ctx->procedureResult()) { + MG_ASSERT(result->variable().size() == 1 || result->variable().size() == 2); + call_proc->result_fields_.push_back(std::any_cast(result->variable()[0]->accept(this))); + std::string result_alias; + if (result->variable().size() == 2) { + result_alias = std::any_cast(result->variable()[1]->accept(this)); + } else { + result_alias = std::any_cast(result->variable()[0]->accept(this)); + } + call_proc->result_identifiers_.push_back(storage_->Create(result_alias)); + } + } else { + const auto &maybe_found = + procedure::FindProcedure(procedure::gModuleRegistry, call_proc->procedure_name_, utils::NewDeleteResource()); + if (!maybe_found) { + throw SemanticException("There is no procedure named '{}'.", call_proc->procedure_name_); + } + const auto &[module, proc] = *maybe_found; + call_proc->result_fields_.reserve(proc->results.size()); + call_proc->result_identifiers_.reserve(proc->results.size()); + for (const auto &[result_name, desc] : proc->results) { + bool is_deprecated = desc.second; + if (is_deprecated) continue; + call_proc->result_fields_.emplace_back(result_name); + call_proc->result_identifiers_.push_back(storage_->Create(std::string(result_name))); + } + // When we leave the scope, we will release the lock on modules. This means + // that someone may reload the procedure and change its result signature. We + // are fine with this, because if new result fields were added then we yield + // the subset of those and that will appear to a user as if they used the + // procedure before reload. Any subsequent `CALL ... YIELD *` will fetch the + // new fields as well. In case the result signature has had some result + // fields removed, then the query execution will report an error that we are + // yielding missing fields. The user can then just retry the query. } - // When we leave the scope, we will release the lock on modules. This means - // that someone may reload the procedure and change its result signature. We - // are fine with this, because if new result fields were added then we yield - // the subset of those and that will appear to a user as if they used the - // procedure before reload. Any subsequent `CALL ... YIELD *` will fetch the - // new fields as well. In case the result signature has had some result - // fields removed, then the query execution will report an error that we are - // yielding missing fields. The user can then just retry the query. } return call_proc;