From a1775b7930194b60cf9757868a09cfdc055531f7 Mon Sep 17 00:00:00 2001 From: Jan Date: Thu, 3 Oct 2019 00:02:27 +0200 Subject: [PATCH] cleanup some of the internal APIs a bit (#10152) --- arangod/Aql/Condition.cpp | 4 +-- arangod/Aql/Condition.h | 2 +- arangod/Aql/Functions.cpp | 5 ++-- arangod/Aql/IndexNode.cpp | 2 +- arangod/Aql/Query.cpp | 21 ++++++++-------- arangod/Aql/Query.h | 32 +++++++++++------------- arangod/Aql/RestAqlHandler.cpp | 2 +- arangod/Aql/RestAqlHandler.h | 2 +- arangod/Aql/TraversalConditionFinder.cpp | 3 +-- arangod/Aql/TraversalNode.cpp | 13 +++------- arangod/Aql/TraversalNode.h | 6 ++--- lib/Basics/AttributeNameParser.cpp | 12 ++++----- 12 files changed, 46 insertions(+), 58 deletions(-) diff --git a/arangod/Aql/Condition.cpp b/arangod/Aql/Condition.cpp index 4918feefa0..e9fc949736 100644 --- a/arangod/Aql/Condition.cpp +++ b/arangod/Aql/Condition.cpp @@ -556,7 +556,7 @@ void Condition::toVelocyPack(arangodb::velocypack::Builder& builder, bool verbos } /// @brief create a condition from VPack -Condition* Condition::fromVPack(ExecutionPlan* plan, arangodb::velocypack::Slice const& slice) { +std::unique_ptr Condition::fromVPack(ExecutionPlan* plan, arangodb::velocypack::Slice const& slice) { auto condition = std::make_unique(plan->getAst()); if (slice.isObject() && slice.length() != 0) { @@ -568,7 +568,7 @@ Condition* Condition::fromVPack(ExecutionPlan* plan, arangodb::velocypack::Slice condition->_isNormalized = true; condition->_isSorted = false; - return condition.release(); + return condition; } /// @brief clone the condition diff --git a/arangod/Aql/Condition.h b/arangod/Aql/Condition.h index 67a5781a61..d3b7cb4921 100644 --- a/arangod/Aql/Condition.h +++ b/arangod/Aql/Condition.h @@ -131,7 +131,7 @@ class Condition { void toVelocyPack(arangodb::velocypack::Builder&, bool) const; /// @brief create a condition from VPack - static Condition* fromVPack(ExecutionPlan*, arangodb::velocypack::Slice const&); + static std::unique_ptr fromVPack(ExecutionPlan*, arangodb::velocypack::Slice const&); /// @brief clone the condition Condition* clone() const; diff --git a/arangod/Aql/Functions.cpp b/arangod/Aql/Functions.cpp index b2589f021f..76516cedfe 100644 --- a/arangod/Aql/Functions.cpp +++ b/arangod/Aql/Functions.cpp @@ -1077,7 +1077,7 @@ AqlValue dateFromParameters( months m{extractFunctionParameterValue(parameters, 1).toInt64()}; days d{extractFunctionParameterValue(parameters, 2).toInt64()}; - if ((y < years{0}) || (m < months{0}) || (d < days{0})) { + if ((y < years{0} || y > years{9999}) || (m < months{0}) || (d < days{0})) { registerWarning(expressionContext, AFN, TRI_ERROR_QUERY_INVALID_DATE_VALUE); return AqlValue(AqlValueHintNull()); } @@ -1121,7 +1121,6 @@ AqlValue dateFromParameters( } if (asTimestamp) { - // TODO: validate return AqlValue(AqlValueHintInt(time.count())); } return ::timeAqlValue(expressionContext, AFN, tp); @@ -4187,7 +4186,7 @@ AqlValue Functions::Sleep(ExpressionContext* expressionContext, } auto& server = application_features::ApplicationServer::server(); - + double const sleepValue = value.toDouble(); auto now = std::chrono::steady_clock::now(); auto const endTime = now + std::chrono::milliseconds(static_cast(sleepValue * 1000.0)); diff --git a/arangod/Aql/IndexNode.cpp b/arangod/Aql/IndexNode.cpp index 318dfed3d7..e3f2e499c3 100644 --- a/arangod/Aql/IndexNode.cpp +++ b/arangod/Aql/IndexNode.cpp @@ -107,7 +107,7 @@ IndexNode::IndexNode(ExecutionPlan* plan, arangodb::velocypack::Slice const& bas TRI_ERROR_BAD_PARAMETER, "\"condition\" attribute should be an object"); } - _condition.reset(Condition::fromVPack(plan, condition)); + _condition = Condition::fromVPack(plan, condition); TRI_ASSERT(_condition != nullptr); diff --git a/arangod/Aql/Query.cpp b/arangod/Aql/Query.cpp index 9443bbfaed..15945f57f5 100644 --- a/arangod/Aql/Query.cpp +++ b/arangod/Aql/Query.cpp @@ -83,19 +83,19 @@ Query::Query(bool contextOwnedByExterior, TRI_vocbase_t& vocbase, _vocbase(vocbase), _context(nullptr), _queryString(queryString), - _queryBuilder(), _bindParameters(bindParameters), _options(options), _collections(&vocbase), - _state(QueryExecutionState::ValueType::INVALID_STATE), _trx(nullptr), - _warnings(), _startTime(TRI_microtime()), - _part(part), + _queryHash(DontCache), _contextOwnedByExterior(contextOwnedByExterior), _killed(false), _isModificationQuery(false), _preparedV8Context(false), + _queryHashCalculated(false), + _state(QueryExecutionState::ValueType::INVALID_STATE), + _part(part), _executionPhase(ExecutionPhase::INITIALIZE), _sharedState(std::make_shared()) { if (_contextOwnedByExterior) { @@ -163,15 +163,16 @@ Query::Query(bool contextOwnedByExterior, TRI_vocbase_t& vocbase, _queryBuilder(queryStruct), _options(options), _collections(&vocbase), - _state(QueryExecutionState::ValueType::INVALID_STATE), _trx(nullptr), - _warnings(), _startTime(TRI_microtime()), - _part(part), + _queryHash(DontCache), _contextOwnedByExterior(contextOwnedByExterior), _killed(false), _isModificationQuery(false), _preparedV8Context(false), + _queryHashCalculated(false), + _state(QueryExecutionState::ValueType::INVALID_STATE), + _part(part), _executionPhase(ExecutionPhase::INITIALIZE), _sharedState(std::make_shared()) { // populate query options @@ -408,7 +409,7 @@ void Query::prepare(QueryRegistry* registry, SerializationFormat format) { #endif if (plan == nullptr) { - plan.reset(preparePlan()); + plan = preparePlan(); TRI_ASSERT(plan != nullptr); @@ -450,7 +451,7 @@ void Query::prepare(QueryRegistry* registry, SerializationFormat format) { /// execute calls it internally. The purpose of this separate method is /// to be able to only prepare a query from VelocyPack and then store it in the /// QueryRegistry. -ExecutionPlan* Query::preparePlan() { +std::unique_ptr Query::preparePlan() { LOG_TOPIC("9625e", DEBUG, Logger::QUERIES) << TRI_microtime() - _startTime << " " << "Query::prepare" << " this: " << (uintptr_t)this; @@ -561,7 +562,7 @@ ExecutionPlan* Query::preparePlan() { // return the V8 context if we are in one exitContext(); - return plan.release(); + return plan; } /// @brief execute an AQL query diff --git a/arangod/Aql/Query.h b/arangod/Aql/Query.h index ead4d5dd2d..b9495814c6 100644 --- a/arangod/Aql/Query.h +++ b/arangod/Aql/Query.h @@ -26,6 +26,7 @@ #include "Aql/BindParameters.h" #include "Aql/Collections.h" +#include "Aql/ExecutionPlan.h" #include "Aql/ExecutionState.h" #include "Aql/Graphs.h" #include "Aql/QueryExecutionState.h" @@ -38,8 +39,6 @@ #include "Aql/SharedQueryState.h" #include "Aql/types.h" #include "Basics/Common.h" -#include "Basics/ConditionLocker.h" -#include "Basics/ConditionVariable.h" #include "V8Server/V8Context.h" #include "VocBase/voc-types.h" @@ -69,7 +68,6 @@ namespace aql { struct AstNode; class Ast; class ExecutionEngine; -class ExecutionPlan; class Query; struct QueryCacheResultEntry; struct QueryProfile; @@ -309,7 +307,7 @@ class Query { CollectionNameResolver const& resolver(); #ifdef ARANGODB_USE_GOOGLE_TESTS - ExecutionPlan* stealPlan() { return std::move(preparePlan()); } + std::unique_ptr stealPlan() { return preparePlan(); } #endif private: @@ -323,7 +321,7 @@ class Query { /// execute calls it internally. The purpose of this separate method is /// to be able to only prepare a query from VelocyPack and then store it in /// the QueryRegistry. - ExecutionPlan* preparePlan(); + std::unique_ptr preparePlan(); /// @brief log a query void log(); @@ -401,10 +399,6 @@ class Query { /// @brief query execution profile std::unique_ptr _profile; - /// @brief current state the query is in (used for profiling and error - /// messages) - QueryExecutionState::ValueType _state; - /// @brief the ExecutionPlan object, if the query is prepared std::shared_ptr _plan; @@ -426,8 +420,8 @@ class Query { /// @brief query start time double _startTime; - /// @brief the query part - QueryPart const _part; + /// @brief hash for this query. will be calculated only once when needed + mutable uint64_t _queryHash = DontCache; /// @brief whether or not someone else has acquired a V8 context for us bool const _contextOwnedByExterior; @@ -442,6 +436,9 @@ class Query { /// once for this expression /// it needs to be run once before any V8-based function is called bool _preparedV8Context; + + /// @brief whether or not the hash was already calculated + mutable bool _queryHashCalculated; /// Create the result in this builder. It is also used to determine /// if we are continuing the query or of we called @@ -450,6 +447,13 @@ class Query { /// Options for _resultBuilder. Optimally, its lifetime should be linked to /// it, but this is hard to do. std::shared_ptr _resultBuilderOptions; + + /// @brief current state the query is in (used for profiling and error + /// messages) + QueryExecutionState::ValueType _state; + + /// @brief the query part + QueryPart const _part; /// Track in which phase of execution we are, in order to implement /// repeatability. @@ -462,12 +466,6 @@ class Query { /// only populated when the query has generated its result(s) and before /// storing the cache entry in the query cache std::unique_ptr _cacheEntry; - - /// @brief hash for this query. will be calculated only once when needed - mutable uint64_t _queryHash = DontCache; - - /// @brief whether or not the hash was already calculated - mutable bool _queryHashCalculated = false; }; } // namespace aql diff --git a/arangod/Aql/RestAqlHandler.cpp b/arangod/Aql/RestAqlHandler.cpp index e1dbd27dcb..2785acf1db 100644 --- a/arangod/Aql/RestAqlHandler.cpp +++ b/arangod/Aql/RestAqlHandler.cpp @@ -270,7 +270,7 @@ void RestAqlHandler::setupClusterQuery() { bool RestAqlHandler::registerSnippets( VPackSlice const snippetsSlice, VPackSlice const collectionSlice, - VPackSlice const variablesSlice, std::shared_ptr options, + VPackSlice const variablesSlice, std::shared_ptr const& options, std::shared_ptr const& ctx, double const ttl, SerializationFormat format, bool& needToLock, VPackBuilder& answerBuilder) { TRI_ASSERT(answerBuilder.isOpenObject()); diff --git a/arangod/Aql/RestAqlHandler.h b/arangod/Aql/RestAqlHandler.h index 56bbd11e2c..d2e6e9268e 100644 --- a/arangod/Aql/RestAqlHandler.h +++ b/arangod/Aql/RestAqlHandler.h @@ -102,7 +102,7 @@ class RestAqlHandler : public RestVocbaseBaseHandler { bool registerSnippets(arangodb::velocypack::Slice const snippets, arangodb::velocypack::Slice const collections, arangodb::velocypack::Slice const variables, - std::shared_ptr options, + std::shared_ptr const& options, std::shared_ptr const& ctx, double const ttl, aql::SerializationFormat format, bool& needToLock, arangodb::velocypack::Builder& answer); diff --git a/arangod/Aql/TraversalConditionFinder.cpp b/arangod/Aql/TraversalConditionFinder.cpp index 3bd9298721..7abc7fffbe 100644 --- a/arangod/Aql/TraversalConditionFinder.cpp +++ b/arangod/Aql/TraversalConditionFinder.cpp @@ -712,9 +712,8 @@ bool TraversalConditionFinder::before(ExecutionNode* en) { } if (!isEmpty) { - // node->setCondition(_condition.release()); originalFilterConditions->normalize(); - node->setCondition(originalFilterConditions.release()); + node->setCondition(std::move(originalFilterConditions)); // We restart here with an empty condition. // All Filters that have been collected thus far // depend on sth issued by this traverser or later diff --git a/arangod/Aql/TraversalNode.cpp b/arangod/Aql/TraversalNode.cpp index dcd65c3c04..318a3a11d5 100644 --- a/arangod/Aql/TraversalNode.cpp +++ b/arangod/Aql/TraversalNode.cpp @@ -100,7 +100,6 @@ TraversalNode::TraversalNode(ExecutionPlan* plan, size_t id, TRI_vocbase_t* vocb : GraphNode(plan, id, vocbase, direction, graph, std::move(options)), _pathOutVariable(nullptr), _inVariable(nullptr), - _condition(nullptr), _fromCondition(nullptr), _toCondition(nullptr), _pruneExpression(std::move(pruneExpression)) { @@ -169,7 +168,6 @@ TraversalNode::TraversalNode(ExecutionPlan* plan, size_t id, TRI_vocbase_t* vocb _pathOutVariable(nullptr), _inVariable(inVariable), _vertexId(vertexId), - _condition(nullptr), _fromCondition(nullptr), _toCondition(nullptr) {} @@ -177,7 +175,6 @@ TraversalNode::TraversalNode(ExecutionPlan* plan, arangodb::velocypack::Slice co : GraphNode(plan, base), _pathOutVariable(nullptr), _inVariable(nullptr), - _condition(nullptr), _fromCondition(nullptr), _toCondition(nullptr) { // In Vertex @@ -273,11 +270,7 @@ TraversalNode::TraversalNode(ExecutionPlan* plan, arangodb::velocypack::Slice co #endif } -TraversalNode::~TraversalNode() { - if (_condition != nullptr) { - delete _condition; - } -} +TraversalNode::~TraversalNode() {} int TraversalNode::checkIsOutVariable(size_t variableId) const { if (_vertexOutVariable != nullptr && _vertexOutVariable->id == variableId) { @@ -699,7 +692,7 @@ void TraversalNode::prepareOptions() { } /// @brief remember the condition to execute for early traversal abortion. -void TraversalNode::setCondition(arangodb::aql::Condition* condition) { +void TraversalNode::setCondition(std::unique_ptr condition) { arangodb::HashSet varsUsedByCondition; Ast::getReferencedVariables(condition->root(), varsUsedByCondition); @@ -713,7 +706,7 @@ void TraversalNode::setCondition(arangodb::aql::Condition* condition) { } } - _condition = condition; + _condition = std::move(condition); } void TraversalNode::registerCondition(bool isConditionOnEdge, uint64_t conditionLevel, diff --git a/arangod/Aql/TraversalNode.h b/arangod/Aql/TraversalNode.h index af1ebd6bff..da53f3b83e 100644 --- a/arangod/Aql/TraversalNode.h +++ b/arangod/Aql/TraversalNode.h @@ -158,10 +158,10 @@ class TraversalNode : public GraphNode { std::string const getStartVertex() const { return _vertexId; } /// @brief remember the condition to execute for early traversal abortion. - void setCondition(Condition* condition); + void setCondition(std::unique_ptr condition); /// @brief return the condition for the node - Condition* condition() const { return _condition; } + Condition* condition() const { return _condition.get(); } /// @brief which variable? -1 none, 0 Edge, 1 Vertex, 2 path int checkIsOutVariable(size_t variableId) const; @@ -210,7 +210,7 @@ class TraversalNode : public GraphNode { std::string _vertexId; /// @brief early abort traversal conditions: - Condition* _condition; + std::unique_ptr _condition; /// @brief variables that are inside of the condition arangodb::HashSet _conditionVariables; diff --git a/lib/Basics/AttributeNameParser.cpp b/lib/Basics/AttributeNameParser.cpp index d3d2cadad6..fe6cb6cb8b 100644 --- a/lib/Basics/AttributeNameParser.cpp +++ b/lib/Basics/AttributeNameParser.cpp @@ -22,6 +22,7 @@ //////////////////////////////////////////////////////////////////////////////// #include +#include #include #include @@ -166,7 +167,7 @@ void arangodb::basics::TRI_AttributeNamesToString(std::vector con TRI_ASSERT(result.empty()); bool isFirst = true; - for (auto& it : input) { + for (auto const& it : input) { if (!isFirst) { result += "."; } @@ -179,12 +180,9 @@ void arangodb::basics::TRI_AttributeNamesToString(std::vector con } bool arangodb::basics::TRI_AttributeNamesHaveExpansion(std::vector const& input) { - for (auto& it : input) { - if (it.shouldExpand) { - return true; - } - } - return false; + return std::any_of(input.begin(), input.end(), [](AttributeName const& value) { + return value.shouldExpand; + }); } ////////////////////////////////////////////////////////////////////////////////