From 7f3468035fabcc7898bddac7846546d95e5ad821 Mon Sep 17 00:00:00 2001 From: jimingquan Date: Mon, 11 Oct 2021 18:00:21 +0800 Subject: [PATCH 1/5] disable modify same col --- src/graph/validator/MaintainValidator.cpp | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/graph/validator/MaintainValidator.cpp b/src/graph/validator/MaintainValidator.cpp index ca04be664ee..c21224dd8b8 100644 --- a/src/graph/validator/MaintainValidator.cpp +++ b/src/graph/validator/MaintainValidator.cpp @@ -24,13 +24,7 @@ namespace graph { Status SchemaValidator::validateColumns(const std::vector &columnSpecs, meta::cpp2::Schema &schema) { - auto status = Status::OK(); - std::unordered_set nameSet; for (auto &spec : columnSpecs) { - if (nameSet.find(*spec->name()) != nameSet.end()) { - return Status::SemanticError("Duplicate column name `%s'", spec->name()->c_str()); - } - nameSet.emplace(*spec->name()); meta::cpp2::ColumnDef column; auto type = spec->type(); column.set_name(*spec->name()); @@ -147,20 +141,32 @@ Status DescEdgeValidator::toPlan() { Status AlterValidator::alterSchema(const std::vector &schemaOpts, const std::vector &schemaProps) { - for (auto &schemaOpt : schemaOpts) { + std::unordered_set uniqueColName; + for (const auto &schemaOpt : schemaOpts) { meta::cpp2::AlterSchemaItem schemaItem; auto opType = schemaOpt->toType(); schemaItem.set_op(opType); meta::cpp2::Schema schema; + if (opType == meta::cpp2::AlterSchemaOp::DROP) { const auto &colNames = schemaOpt->columnNames(); for (auto &colName : colNames) { + if (uniqueColName.find(colName) != uniqueColName.end()) { + return Status::SemanticError("Duplicate column name `%s'", colName.c_str()); + } + uniqueColName.emplace(colName); meta::cpp2::ColumnDef column; column.name = *colName; schema.columns_ref().value().emplace_back(std::move(column)); } } else { const auto &specs = schemaOpt->columnSpecs(); + for (auto &spec : specs) { + if (uniqueColName.find(*spec->name()) != uniqueColName.end()) { + return Status::SemanticError("Duplicate column name `%s'", spec->name()->c_str()); + } + uniqueColName.emplace(*spec->name()); + } NG_LOG_AND_RETURN_IF_ERROR(validateColumns(specs, schema)); } From f953f713450454e1d779fbb9156c51ec1a8702bc Mon Sep 17 00:00:00 2001 From: jimingquan Date: Mon, 11 Oct 2021 19:32:04 +0800 Subject: [PATCH 2/5] add test case --- src/graph/validator/MaintainValidator.cpp | 6 ++-- tests/tck/features/ttl/TTL.feature | 44 +++++++++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/src/graph/validator/MaintainValidator.cpp b/src/graph/validator/MaintainValidator.cpp index c21224dd8b8..b58b1e34848 100644 --- a/src/graph/validator/MaintainValidator.cpp +++ b/src/graph/validator/MaintainValidator.cpp @@ -151,10 +151,10 @@ Status AlterValidator::alterSchema(const std::vector &sche if (opType == meta::cpp2::AlterSchemaOp::DROP) { const auto &colNames = schemaOpt->columnNames(); for (auto &colName : colNames) { - if (uniqueColName.find(colName) != uniqueColName.end()) { - return Status::SemanticError("Duplicate column name `%s'", colName.c_str()); + if (uniqueColName.find(*colName) != uniqueColName.end()) { + return Status::SemanticError("Duplicate column name `%s'", colName->c_str()); } - uniqueColName.emplace(colName); + uniqueColName.emplace(*colName); meta::cpp2::ColumnDef column; column.name = *colName; schema.columns_ref().value().emplace_back(std::move(column)); diff --git a/tests/tck/features/ttl/TTL.feature b/tests/tck/features/ttl/TTL.feature index ffd176b79f4..aebc2ab1b8e 100644 --- a/tests/tck/features/ttl/TTL.feature +++ b/tests/tck/features/ttl/TTL.feature @@ -326,6 +326,50 @@ Feature: TTLTest Then the result should be, in any order: | Edge | Create Edge | | "work2" | 'CREATE EDGE `work2` (\n `email` string NULL,\n `age` string NULL,\n `gender` string NULL\n) ttl_duration = 0, ttl_col = ""' | + When executing query: + """ + CREATE EDGE player(id int, name string, age int, address string, score float); + """ + Then the execution should be successful + When executing query: + """ + SHOW CREATE EDGE player; + """ + Then the result should be, in any order: + | Edge | Create Edge | + | "player" | 'CREATE EDGE `player` (\n `id` int64 NULL,\n `name` string NULL,\n `age` int64 NULL,\n `address` string NULL,\n `score` float NULL\n) ttl_duration = 0, ttl_col = ""' | + When executing query: + """ + ALTER EDGE player change(name int), drop(name); + """ + Then a SemanticError should be raised at runtime: Duplicate column name `name' + When executing query: + """ + ALTER EDGE player drop(name), change(name int); + """ + Then a SemanticError should be raised at runtime: Duplicate column name `name' + When executing query: + """ + ALTER EDGE player drop(name, name), change(address int); + """ + Then a SemanticError should be raised at runtime: Duplicate column name `name' + When executing query: + """ + ALTER EDGE player change(address int, address string); + """ + Then a SemanticError should be raised at runtime: Duplicate column name `address' + When executing query: + """ + ALTER EDGE player change(address int), drop(name); + """ + Then the execution should be successful + When executing query: + """ + SHOW CREATE EDGE player; + """ + Then the result should be, in any order: + | Edge | Create Edge | + | "player" | 'CREATE EDGE `player` (\n `id` int64 NULL,\n `age` int64 NULL,\n `address` int64 NULL,\n `score` float NULL\n) ttl_duration = 0, ttl_col = ""' | And drop the used space Scenario: TTLTest Datatest From 67c908b78c3dc9bf107720a337b39222d631bc7e Mon Sep 17 00:00:00 2001 From: jimingquan Date: Wed, 13 Oct 2021 14:16:45 +0800 Subject: [PATCH 3/5] refactor ddl --- src/graph/context/ast/QueryAstContext.h | 11 + src/graph/planner/CMakeLists.txt | 1 + src/graph/planner/PlannersRegister.cpp | 21 ++ src/graph/planner/PlannersRegister.h | 1 + src/graph/planner/ngql/MaintainPlanner.cpp | 66 +++++ src/graph/planner/ngql/MaintainPlanner.h | 62 +++++ src/graph/validator/FetchEdgesValidator.cpp | 4 +- src/graph/validator/GroupByValidator.cpp | 2 +- src/graph/validator/LookupValidator.cpp | 2 +- src/graph/validator/MaintainValidator.cpp | 254 ++++++++++---------- src/graph/validator/MaintainValidator.h | 64 ++--- src/graph/validator/MatchValidator.cpp | 4 +- src/graph/validator/MutateValidator.cpp | 10 +- src/graph/validator/Validator.cpp | 8 +- src/graph/validator/Validator.h | 2 - 15 files changed, 316 insertions(+), 196 deletions(-) create mode 100644 src/graph/planner/ngql/MaintainPlanner.cpp create mode 100644 src/graph/planner/ngql/MaintainPlanner.h diff --git a/src/graph/context/ast/QueryAstContext.h b/src/graph/context/ast/QueryAstContext.h index fed455a2198..a1e09dab85c 100644 --- a/src/graph/context/ast/QueryAstContext.h +++ b/src/graph/context/ast/QueryAstContext.h @@ -157,6 +157,17 @@ struct FetchEdgesContext final : public AstContext { std::string inputVarName; }; +struct AlterContext final : public AstContext { + std::vector schemaItems; + meta::cpp2::SchemaProp schemaProps; +}; + +struct CreateContext final : public AstContext { + bool ifNotExist{false}; + std::string name; + meta::cpp2::Schema schema; +}; + } // namespace graph } // namespace nebula #endif // GRAPH_CONTEXT_AST_QUERYASTCONTEXT_H_ diff --git a/src/graph/planner/CMakeLists.txt b/src/graph/planner/CMakeLists.txt index 47d76119229..5f822cafd73 100644 --- a/src/graph/planner/CMakeLists.txt +++ b/src/graph/planner/CMakeLists.txt @@ -42,4 +42,5 @@ nebula_add_library( ngql/LookupPlanner.cpp ngql/FetchVerticesPlanner.cpp ngql/FetchEdgesPlanner.cpp + ngql/MaintainPlanner.cpp ) diff --git a/src/graph/planner/PlannersRegister.cpp b/src/graph/planner/PlannersRegister.cpp index 3cdea03b920..15b3d4064d8 100644 --- a/src/graph/planner/PlannersRegister.cpp +++ b/src/graph/planner/PlannersRegister.cpp @@ -17,6 +17,7 @@ #include "graph/planner/ngql/FetchVerticesPlanner.h" #include "graph/planner/ngql/GoPlanner.h" #include "graph/planner/ngql/LookupPlanner.h" +#include "graph/planner/ngql/MaintainPlanner.h" #include "graph/planner/ngql/PathPlanner.h" #include "graph/planner/ngql/SubgraphPlanner.h" @@ -24,10 +25,30 @@ namespace nebula { namespace graph { void PlannersRegister::registPlanners() { + registDDL(); registSequential(); registMatch(); } +void PlannersRegister::registDDL() { + { + auto& planners = Planner::plannersMap()[Sentence::Kind::kAlterTag]; + planners.emplace_back(&AlterTagPlanner::match, &AlterTagPlanner::make); + } + { + auto& planners = Planner::plannersMap()[Sentence::Kind::kAlterEdge]; + planners.emplace_back(&AlterEdgePlanner::match, &AlterEdgePlanner::make); + } + { + auto& planners = Planner::plannersMap()[Sentence::Kind::kCreateTag]; + planners.emplace_back(&CreateTagPlanner::match, &CreateTagPlanner::make); + } + { + auto& planners = Planner::plannersMap()[Sentence::Kind::kCreateEdge]; + planners.emplace_back(&CreateEdgePlanner::match, &CreateEdgePlanner::make); + } +} + void PlannersRegister::registSequential() { { auto& planners = Planner::plannersMap()[Sentence::Kind::kSequential]; diff --git a/src/graph/planner/PlannersRegister.h b/src/graph/planner/PlannersRegister.h index 995a4a3615e..bc89cac3870 100644 --- a/src/graph/planner/PlannersRegister.h +++ b/src/graph/planner/PlannersRegister.h @@ -18,6 +18,7 @@ class PlannersRegister final { static void registPlanners(); private: + static void registDDL(); static void registSequential(); static void registMatch(); }; diff --git a/src/graph/planner/ngql/MaintainPlanner.cpp b/src/graph/planner/ngql/MaintainPlanner.cpp new file mode 100644 index 00000000000..02d009e6432 --- /dev/null +++ b/src/graph/planner/ngql/MaintainPlanner.cpp @@ -0,0 +1,66 @@ +/* Copyright (c) 2021 vesoft inc. All rights reserved. + * + * This source code is licensed under Apache 2.0 License, + * attached with Common Clause Condition 1.0, found in the LICENSES directory. + */ + +#include "graph/planner/ngql/MaintainPlanner.h" + +#include "graph/context/ast/QueryAstContext.h" +#include "graph/planner/plan/Maintain.h" + +namespace nebula { +namespace graph { + +StatusOr CreateTagPlanner::transform(AstContext* astCtx) { + auto createCtx = static_cast(astCtx); + SubPlan plan; + plan.root = plan.tail = CreateTag::make(createCtx->qctx, + nullptr, + std::move(createCtx->name), + std::move(createCtx->schema), + createCtx->ifNotExist); + return plan; +} + +StatusOr CreateEdgePlanner::transform(AstContext* astCtx) { + auto createCtx = static_cast(astCtx); + SubPlan plan; + plan.root = plan.tail = CreateEdge::make(createCtx->qctx, + nullptr, + std::move(createCtx->name), + std::move(createCtx->schema), + createCtx->ifNotExist); + return plan; +} + +StatusOr AlterTagPlanner::transform(AstContext* astCtx) { + auto alterCtx = static_cast(astCtx); + auto qctx = alterCtx->qctx; + auto name = *static_cast(alterCtx->sentence)->name(); + SubPlan plan; + plan.root = plan.tail = AlterTag::make(qctx, + nullptr, + alterCtx->space.id, + std::move(name), + std::move(alterCtx->schemaItems), + std::move(alterCtx->schemaProps)); + return plan; +} + +StatusOr AlterEdgePlanner::transform(AstContext* astCtx) { + auto alterCtx = static_cast(astCtx); + auto qctx = alterCtx->qctx; + auto name = *static_cast(alterCtx->sentence)->name(); + SubPlan plan; + plan.root = plan.tail = AlterEdge::make(qctx, + nullptr, + alterCtx->space.id, + std::move(name), + std::move(alterCtx->schemaItems), + std::move(alterCtx->schemaProps)); + return plan; +} + +} // namespace graph +} // namespace nebula diff --git a/src/graph/planner/ngql/MaintainPlanner.h b/src/graph/planner/ngql/MaintainPlanner.h new file mode 100644 index 00000000000..eb8a8953652 --- /dev/null +++ b/src/graph/planner/ngql/MaintainPlanner.h @@ -0,0 +1,62 @@ +/* Copyright (c) 2021 vesoft inc. All rights reserved. + * + * This source code is licensed under Apache 2.0 License, + * attached with Common Clause Condition 1.0, found in the LICENSES directory. + */ +#pragma once + +#include "common/base/Base.h" +#include "graph/planner/Planner.h" +#include "graph/planner/plan/PlanNode.h" + +namespace nebula { +namespace graph { + +struct AstContext; + +class CreateTagPlanner final : public Planner { + public: + static std::unique_ptr make() { + return std::unique_ptr(new CreateTagPlanner()); + } + static bool match(AstContext* astCtx) { + return astCtx->sentence->kind() == Sentence::Kind::kCreateTag; + } + StatusOr transform(AstContext* astCtx) override; +}; + +class CreateEdgePlanner final : public Planner { + public: + static std::unique_ptr make() { + return std::unique_ptr(new CreateEdgePlanner()); + } + static bool match(AstContext* astCtx) { + return astCtx->sentence->kind() == Sentence::Kind::kCreateEdge; + } + StatusOr transform(AstContext* astCtx) override; +}; + +class AlterTagPlanner final : public Planner { + public: + static std::unique_ptr make() { + return std::unique_ptr(new AlterTagPlanner()); + } + static bool match(AstContext* astCtx) { + return astCtx->sentence->kind() == Sentence::Kind::kAlterTag; + } + StatusOr transform(AstContext* astCtx) override; +}; + +class AlterEdgePlanner final : public Planner { + public: + static std::unique_ptr make() { + return std::unique_ptr(new AlterEdgePlanner()); + } + static bool match(AstContext* astCtx) { + return astCtx->sentence->kind() == Sentence::Kind::kAlterEdge; + } + StatusOr transform(AstContext* astCtx) override; +}; + +} // namespace graph +} // namespace nebula diff --git a/src/graph/validator/FetchEdgesValidator.cpp b/src/graph/validator/FetchEdgesValidator.cpp index 88bd8c33cad..0dc826c9650 100644 --- a/src/graph/validator/FetchEdgesValidator.cpp +++ b/src/graph/validator/FetchEdgesValidator.cpp @@ -105,7 +105,7 @@ Status FetchEdgesValidator::validateEdgeKey() { auto keys = sentence->keys()->keys(); edgeKeys.rows.reserve(keys.size()); for (const auto &key : keys) { - if (!evaluableExpr(key->srcid())) { + if (!ExpressionUtils::isEvaluableExpr(key->srcid())) { return Status::SemanticError("`%s' is not evaluable.", key->srcid()->toString().c_str()); } auto src = key->srcid()->eval(ctx); @@ -116,7 +116,7 @@ Status FetchEdgesValidator::validateEdgeKey() { } auto ranking = key->rank(); - if (!evaluableExpr(key->dstid())) { + if (!ExpressionUtils::isEvaluableExpr(key->dstid())) { return Status::SemanticError("`%s' is not evaluable.", key->dstid()->toString().c_str()); } auto dst = key->dstid()->eval(ctx); diff --git a/src/graph/validator/GroupByValidator.cpp b/src/graph/validator/GroupByValidator.cpp index 379d7a5ff82..42cf15151de 100644 --- a/src/graph/validator/GroupByValidator.cpp +++ b/src/graph/validator/GroupByValidator.cpp @@ -174,7 +174,7 @@ Status GroupByValidator::groupClauseSemanticCheck() { return false; }; for (auto* expr : yieldCols_) { - if (evaluableExpr(expr)) { + if (ExpressionUtils::isEvaluableExpr(expr)) { continue; } FindVisitor visitor(finder); diff --git a/src/graph/validator/LookupValidator.cpp b/src/graph/validator/LookupValidator.cpp index 8d4f421d6ba..f88e85e8d9c 100644 --- a/src/graph/validator/LookupValidator.cpp +++ b/src/graph/validator/LookupValidator.cpp @@ -340,7 +340,7 @@ StatusOr LookupValidator::checkConstExpr(Expression* expr, const std::string& prop, const ExprKind kind) { auto* pool = expr->getObjPool(); - if (!evaluableExpr(expr)) { + if (!ExpressionUtils::isEvaluableExpr(expr)) { return Status::SemanticError("'%s' is not an evaluable expression.", expr->toString().c_str()); } auto schemaMgr = qctx_->schemaMng(); diff --git a/src/graph/validator/MaintainValidator.cpp b/src/graph/validator/MaintainValidator.cpp index b58b1e34848..e463a2c01ba 100644 --- a/src/graph/validator/MaintainValidator.cpp +++ b/src/graph/validator/MaintainValidator.cpp @@ -22,8 +22,8 @@ namespace nebula { namespace graph { -Status SchemaValidator::validateColumns(const std::vector &columnSpecs, - meta::cpp2::Schema &schema) { +static Status validateColumns(const std::vector &columnSpecs, + meta::cpp2::Schema &schema) { for (auto &spec : columnSpecs) { meta::cpp2::ColumnDef column; auto type = spec->type(); @@ -36,13 +36,12 @@ Status SchemaValidator::validateColumns(const std::vector if (property->isNullable()) { column.set_nullable(property->nullable()); } else if (property->isDefaultValue()) { - if (!evaluableExpr(property->defaultValue())) { + if (!ExpressionUtils::isEvaluableExpr(property->defaultValue())) { return Status::SemanticError("Wrong default value experssion `%s'", property->defaultValue()->toString().c_str()); } auto *defaultValueExpr = property->defaultValue(); - // some expression is evaluable but not pure so only fold instead of - // eval here + // some expression is evaluable but not pure so only fold instead of eval here auto foldRes = ExpressionUtils::foldConstantExpr(defaultValueExpr); NG_RETURN_IF_ERROR(foldRes); column.set_default_value(foldRes.value()->encode()); @@ -55,92 +54,12 @@ Status SchemaValidator::validateColumns(const std::vector } schema.columns_ref().value().emplace_back(std::move(column)); } - - return Status::OK(); -} - -Status CreateTagValidator::validateImpl() { - auto sentence = static_cast(sentence_); - name_ = *sentence->name(); - ifNotExist_ = sentence->isIfNotExist(); - - // Check the validateContext has the same name schema - auto pro = vctx_->getSchema(name_); - if (pro != nullptr) { - return Status::SemanticError("Has the same name `%s' in the SequentialSentences", - name_.c_str()); - } - NG_RETURN_IF_ERROR(validateColumns(sentence->columnSpecs(), schema_)); - NG_RETURN_IF_ERROR(SchemaUtil::validateProps(sentence->getSchemaProps(), schema_)); - // Save the schema in validateContext - auto pool = qctx_->objPool(); - auto schemaPro = SchemaUtil::generateSchemaProvider(pool, 0, schema_); - vctx_->addSchema(name_, schemaPro); - return Status::OK(); -} - -Status CreateTagValidator::toPlan() { - auto *plan = qctx_->plan(); - auto doNode = - CreateTag::make(qctx_, plan->root(), std::move(name_), std::move(schema_), ifNotExist_); - root_ = doNode; - tail_ = root_; - return Status::OK(); -} - -Status CreateEdgeValidator::validateImpl() { - auto sentence = static_cast(sentence_); - auto status = Status::OK(); - name_ = *sentence->name(); - ifNotExist_ = sentence->isIfNotExist(); - // Check the validateContext has the same name schema - auto pro = vctx_->getSchema(name_); - if (pro != nullptr) { - return Status::SemanticError("Has the same name `%s' in the SequentialSentences", - name_.c_str()); - } - NG_RETURN_IF_ERROR(validateColumns(sentence->columnSpecs(), schema_)); - NG_RETURN_IF_ERROR(SchemaUtil::validateProps(sentence->getSchemaProps(), schema_)); - // Save the schema in validateContext - auto pool = qctx_->objPool(); - auto schemaPro = SchemaUtil::generateSchemaProvider(pool, 0, schema_); - vctx_->addSchema(name_, schemaPro); - return Status::OK(); -} - -Status CreateEdgeValidator::toPlan() { - auto *plan = qctx_->plan(); - auto doNode = - CreateEdge::make(qctx_, plan->root(), std::move(name_), std::move(schema_), ifNotExist_); - root_ = doNode; - tail_ = root_; - return Status::OK(); -} - -Status DescTagValidator::validateImpl() { return Status::OK(); } - -Status DescTagValidator::toPlan() { - auto sentence = static_cast(sentence_); - auto name = *sentence->name(); - auto doNode = DescTag::make(qctx_, nullptr, std::move(name)); - root_ = doNode; - tail_ = root_; - return Status::OK(); -} - -Status DescEdgeValidator::validateImpl() { return Status::OK(); } - -Status DescEdgeValidator::toPlan() { - auto sentence = static_cast(sentence_); - auto name = *sentence->name(); - auto doNode = DescEdge::make(qctx_, nullptr, std::move(name)); - root_ = doNode; - tail_ = root_; return Status::OK(); } -Status AlterValidator::alterSchema(const std::vector &schemaOpts, - const std::vector &schemaProps) { +static StatusOr> validateSchemaOpts( + const std::vector &schemaOpts) { + std::vector schemaItems; std::unordered_set uniqueColName; for (const auto &schemaOpt : schemaOpts) { meta::cpp2::AlterSchemaItem schemaItem; @@ -171,76 +90,149 @@ Status AlterValidator::alterSchema(const std::vector &sche } schemaItem.set_schema(std::move(schema)); - schemaItems_.emplace_back(std::move(schemaItem)); + schemaItems.emplace_back(std::move(schemaItem)); } + return schemaItems; +} - for (auto &schemaProp : schemaProps) { - auto propType = schemaProp->getPropType(); - StatusOr retInt; - StatusOr retStr; - int ttlDuration; +static StatusOr validateSchemaProps( + const std::vector &schemaProps) { + meta::cpp2::SchemaProp schemaProp; + for (const auto &prop : schemaProps) { + auto propType = prop->getPropType(); switch (propType) { - case SchemaPropItem::TTL_DURATION: - retInt = schemaProp->getTtlDuration(); - NG_RETURN_IF_ERROR(retInt); - ttlDuration = retInt.value(); - schemaProp_.set_ttl_duration(ttlDuration); + case SchemaPropItem::TTL_DURATION: { + auto ttlDur = prop->getTtlDuration(); + NG_RETURN_IF_ERROR(ttlDur); + schemaProp.set_ttl_duration(ttlDur.value()); break; - case SchemaPropItem::TTL_COL: + } + case SchemaPropItem::TTL_COL: { // Check the legality of the column in meta - retStr = schemaProp->getTtlCol(); - NG_RETURN_IF_ERROR(retStr); - schemaProp_.set_ttl_col(retStr.value()); + auto ttlCol = prop->getTtlCol(); + NG_RETURN_IF_ERROR(ttlCol); + schemaProp.set_ttl_col(ttlCol.value()); break; - case SchemaPropItem::COMMENT: + } + case SchemaPropItem::COMMENT: { // Check the legality of the column in meta - retStr = schemaProp->getComment(); - NG_RETURN_IF_ERROR(retStr); - schemaProp_.set_comment(retStr.value()); + auto comment = prop->getComment(); + NG_RETURN_IF_ERROR(comment); + schemaProp.set_comment(comment.value()); break; - default: + } + default: { return Status::SemanticError("Property type not support"); + } } } + return schemaProp; +} + +static Status checkColName(const std::vector specs) { + std::unordered_set uniqueColName; + for (const auto &spec : specs) { + auto name = *spec->name(); + if (uniqueColName.find(name) != uniqueColName.end()) { + return Status::SemanticError("Duplicate column name `%s'", name.c_str()); + } + uniqueColName.emplace(name); + } return Status::OK(); } -Status AlterTagValidator::validateImpl() { - auto sentence = static_cast(sentence_); - name_ = *sentence->name(); - return alterSchema(sentence->getSchemaOpts(), sentence->getSchemaProps()); +Status CreateTagValidator::validateImpl() { + createCtx_ = getContext(); + auto sentence = static_cast(sentence_); + createCtx_->ifNotExist = sentence->isIfNotExist(); + auto name = *sentence->name(); + // Check the validateContext has the same name schema + auto pro = vctx_->getSchema(name); + if (pro != nullptr) { + return Status::SemanticError("Has the same name `%s' in the SequentialSentences", name.c_str()); + } + meta::cpp2::Schema schema; + NG_RETURN_IF_ERROR(checkColName(sentence->columnSpecs())); + NG_RETURN_IF_ERROR(validateColumns(sentence->columnSpecs(), schema)); + NG_RETURN_IF_ERROR(SchemaUtil::validateProps(sentence->getSchemaProps(), schema)); + // Save the schema in validateContext + auto pool = qctx_->objPool(); + auto schemaPro = SchemaUtil::generateSchemaProvider(pool, 0, schema); + vctx_->addSchema(name, schemaPro); + createCtx_->name = std::move(name); + createCtx_->schema = std::move(schema); + return Status::OK(); +} + +Status CreateEdgeValidator::validateImpl() { + createCtx_ = getContext(); + auto sentence = static_cast(sentence_); + createCtx_->ifNotExist = sentence->isIfNotExist(); + auto name = *sentence->name(); + // Check the validateContext has the same name schema + auto pro = vctx_->getSchema(name); + if (pro != nullptr) { + return Status::SemanticError("Has the same name `%s' in the SequentialSentences", name.c_str()); + } + meta::cpp2::Schema schema; + NG_RETURN_IF_ERROR(checkColName(sentence->columnSpecs())); + NG_RETURN_IF_ERROR(validateColumns(sentence->columnSpecs(), schema)); + NG_RETURN_IF_ERROR(SchemaUtil::validateProps(sentence->getSchemaProps(), schema)); + // Save the schema in validateContext + auto pool = qctx_->objPool(); + auto schemaPro = SchemaUtil::generateSchemaProvider(pool, 0, schema); + vctx_->addSchema(name, schemaPro); + createCtx_->name = std::move(name); + createCtx_->schema = std::move(schema); + return Status::OK(); } -Status AlterTagValidator::toPlan() { - auto *doNode = AlterTag::make(qctx_, - nullptr, - vctx_->whichSpace().id, - std::move(name_), - std::move(schemaItems_), - std::move(schemaProp_)); +Status DescTagValidator::validateImpl() { return Status::OK(); } + +Status DescTagValidator::toPlan() { + auto sentence = static_cast(sentence_); + auto name = *sentence->name(); + auto doNode = DescTag::make(qctx_, nullptr, std::move(name)); root_ = doNode; tail_ = root_; return Status::OK(); } -Status AlterEdgeValidator::validateImpl() { - auto sentence = static_cast(sentence_); - name_ = *sentence->name(); - return alterSchema(sentence->getSchemaOpts(), sentence->getSchemaProps()); -} +Status DescEdgeValidator::validateImpl() { return Status::OK(); } -Status AlterEdgeValidator::toPlan() { - auto *doNode = AlterEdge::make(qctx_, - nullptr, - vctx_->whichSpace().id, - std::move(name_), - std::move(schemaItems_), - std::move(schemaProp_)); +Status DescEdgeValidator::toPlan() { + auto sentence = static_cast(sentence_); + auto name = *sentence->name(); + auto doNode = DescEdge::make(qctx_, nullptr, std::move(name)); root_ = doNode; tail_ = root_; return Status::OK(); } +Status AlterTagValidator::validateImpl() { + alterCtx_ = getContext(); + auto sentence = static_cast(sentence_); + auto schemaItems = validateSchemaOpts(sentence->getSchemaOpts()); + NG_RETURN_IF_ERROR(schemaItems); + alterCtx_->schemaItems = std::move(schemaItems.value()); + auto schemaProps = validateSchemaProps(sentence->getSchemaProps()); + NG_RETURN_IF_ERROR(schemaProps); + alterCtx_->schemaProps = std::move(schemaProps.value()); + return Status::OK(); +} + +Status AlterEdgeValidator::validateImpl() { + alterCtx_ = getContext(); + auto sentence = static_cast(sentence_); + auto schemaItems = validateSchemaOpts(sentence->getSchemaOpts()); + NG_RETURN_IF_ERROR(schemaItems); + alterCtx_->schemaItems = std::move(schemaItems.value()); + auto schemaProps = validateSchemaProps(sentence->getSchemaProps()); + NG_RETURN_IF_ERROR(schemaProps); + alterCtx_->schemaProps = std::move(schemaProps.value()); + return Status::OK(); +} + Status ShowTagsValidator::validateImpl() { return Status::OK(); } Status ShowTagsValidator::toPlan() { diff --git a/src/graph/validator/MaintainValidator.h b/src/graph/validator/MaintainValidator.h index 3c0dc912f8f..c3d88fa8e2a 100644 --- a/src/graph/validator/MaintainValidator.h +++ b/src/graph/validator/MaintainValidator.h @@ -8,50 +8,35 @@ #define GRAPH_VALIDATOR_MAINTAINVALIDATOR_H_ #include "clients/meta/MetaClient.h" +#include "graph/context/ast/QueryAstContext.h" #include "graph/validator/Validator.h" #include "parser/AdminSentences.h" #include "parser/MaintainSentences.h" namespace nebula { namespace graph { -class SchemaValidator : public Validator { +class CreateTagValidator final : public Validator { public: - SchemaValidator(Sentence* sentence, QueryContext* context) : Validator(sentence, context) {} + CreateTagValidator(Sentence* sentence, QueryContext* context) : Validator(sentence, context) {} - protected: - Status validateColumns(const std::vector& columnSpecs, - meta::cpp2::Schema& schema); - - protected: - std::string name_; -}; - -class CreateTagValidator final : public SchemaValidator { - public: - CreateTagValidator(Sentence* sentence, QueryContext* context) - : SchemaValidator(sentence, context) {} + AstContext* getAstContext() override { return createCtx_.get(); } private: Status validateImpl() override; - Status toPlan() override; - private: - meta::cpp2::Schema schema_; - bool ifNotExist_; + std::unique_ptr createCtx_; }; -class CreateEdgeValidator final : public SchemaValidator { +class CreateEdgeValidator final : public Validator { public: - CreateEdgeValidator(Sentence* sentence, QueryContext* context) - : SchemaValidator(sentence, context) {} + CreateEdgeValidator(Sentence* sentence, QueryContext* context) : Validator(sentence, context) {} + + AstContext* getAstContext() override { return createCtx_.get(); } private: Status validateImpl() override; - Status toPlan() override; - private: - meta::cpp2::Schema schema_; - bool ifNotExist_; + std::unique_ptr createCtx_; }; class DescTagValidator final : public Validator { @@ -96,39 +81,28 @@ class ShowCreateEdgeValidator final : public Validator { Status toPlan() override; }; -class AlterValidator : public SchemaValidator { +class AlterTagValidator final : public Validator { public: - AlterValidator(Sentence* sentence, QueryContext* context) : SchemaValidator(sentence, context) {} + AlterTagValidator(Sentence* sentence, QueryContext* context) : Validator(sentence, context) {} - protected: - Status alterSchema(const std::vector& schemaOpts, - const std::vector& schemaProps); - - protected: - std::vector schemaItems_; - meta::cpp2::SchemaProp schemaProp_; -}; - -class AlterTagValidator final : public AlterValidator { - public: - AlterTagValidator(Sentence* sentence, QueryContext* context) - : AlterValidator(sentence, context) {} + AstContext* getAstContext() override { return alterCtx_.get(); } private: Status validateImpl() override; - Status toPlan() override; + std::unique_ptr alterCtx_; }; -class AlterEdgeValidator final : public AlterValidator { +class AlterEdgeValidator final : public Validator { public: - AlterEdgeValidator(Sentence* sentence, QueryContext* context) - : AlterValidator(sentence, context) {} + AlterEdgeValidator(Sentence* sentence, QueryContext* context) : Validator(sentence, context) {} + + AstContext* getAstContext() override { return alterCtx_.get(); } private: Status validateImpl() override; - Status toPlan() override; + std::unique_ptr alterCtx_; }; class ShowTagsValidator final : public Validator { diff --git a/src/graph/validator/MatchValidator.cpp b/src/graph/validator/MatchValidator.cpp index aae111617b9..f090eacc72f 100644 --- a/src/graph/validator/MatchValidator.cpp +++ b/src/graph/validator/MatchValidator.cpp @@ -618,7 +618,7 @@ Status MatchValidator::validatePagination(const Expression *skipExpr, int64_t skip = 0; int64_t limit = std::numeric_limits::max(); if (skipExpr != nullptr) { - if (!evaluableExpr(skipExpr)) { + if (!ExpressionUtils::isEvaluableExpr(skipExpr)) { return Status::SemanticError("SKIP should be instantly evaluable"); } QueryExpressionContext ctx; @@ -633,7 +633,7 @@ Status MatchValidator::validatePagination(const Expression *skipExpr, } if (limitExpr != nullptr) { - if (!evaluableExpr(limitExpr)) { + if (!ExpressionUtils::isEvaluableExpr(limitExpr)) { return Status::SemanticError("SKIP should be instantly evaluable"); } QueryExpressionContext ctx; diff --git a/src/graph/validator/MutateValidator.cpp b/src/graph/validator/MutateValidator.cpp index 05643548fc3..170569e7667 100644 --- a/src/graph/validator/MutateValidator.cpp +++ b/src/graph/validator/MutateValidator.cpp @@ -89,7 +89,7 @@ Status InsertVerticesValidator::prepareVertices() { if (propSize_ != row->values().size()) { return Status::SemanticError("Column count doesn't match value count."); } - if (!evaluableExpr(row->id())) { + if (!ExpressionUtils::isEvaluableExpr(row->id())) { LOG(ERROR) << "Wrong vid expression `" << row->id()->toString() << "\""; return Status::SemanticError("Wrong vid expression `%s'", row->id()->toString().c_str()); } @@ -99,7 +99,7 @@ Status InsertVerticesValidator::prepareVertices() { // check value expr for (auto &value : row->values()) { - if (!evaluableExpr(value)) { + if (!ExpressionUtils::isEvaluableExpr(value)) { LOG(ERROR) << "Insert wrong value: `" << value->toString() << "'."; return Status::SemanticError("Insert wrong value: `%s'.", value->toString().c_str()); } @@ -203,13 +203,13 @@ Status InsertEdgesValidator::prepareEdges() { if (propNames_.size() != row->values().size()) { return Status::SemanticError("Column count doesn't match value count."); } - if (!evaluableExpr(row->srcid())) { + if (!ExpressionUtils::isEvaluableExpr(row->srcid())) { LOG(ERROR) << "Wrong src vid expression `" << row->srcid()->toString() << "\""; return Status::SemanticError("Wrong src vid expression `%s'", row->srcid()->toString().c_str()); } - if (!evaluableExpr(row->dstid())) { + if (!ExpressionUtils::isEvaluableExpr(row->dstid())) { LOG(ERROR) << "Wrong dst vid expression `" << row->dstid()->toString() << "\""; return Status::SemanticError("Wrong dst vid expression `%s'", row->dstid()->toString().c_str()); @@ -226,7 +226,7 @@ Status InsertEdgesValidator::prepareEdges() { // check value expr for (auto &value : row->values()) { - if (!evaluableExpr(value)) { + if (!ExpressionUtils::isEvaluableExpr(value)) { LOG(ERROR) << "Insert wrong value: `" << value->toString() << "'."; return Status::SemanticError("Insert wrong value: `%s'.", value->toString().c_str()); } diff --git a/src/graph/validator/Validator.cpp b/src/graph/validator/Validator.cpp index 761c0d691f7..48963d4ba4e 100644 --- a/src/graph/validator/Validator.cpp +++ b/src/graph/validator/Validator.cpp @@ -373,12 +373,6 @@ Status Validator::deduceProps(const Expression* expr, ExpressionProps& exprProps return std::move(visitor).status(); } -bool Validator::evaluableExpr(const Expression* expr) const { - EvaluableExprVisitor visitor; - const_cast(expr)->accept(&visitor); - return visitor.ok(); -} - Status Validator::toPlan() { auto* astCtx = getAstContext(); if (astCtx != nullptr) { @@ -458,7 +452,7 @@ Status Validator::validateStarts(const VerticesClause* clause, Starts& starts) { auto vidList = clause->vidList(); QueryExpressionContext ctx; for (auto* expr : vidList) { - if (!evaluableExpr(expr)) { + if (!ExpressionUtils::isEvaluableExpr(expr)) { return Status::SemanticError("`%s' is not an evaluable expression.", expr->toString().c_str()); } diff --git a/src/graph/validator/Validator.h b/src/graph/validator/Validator.h index 4d1320ff14b..381b1db65fe 100644 --- a/src/graph/validator/Validator.h +++ b/src/graph/validator/Validator.h @@ -110,8 +110,6 @@ class Validator { Status deduceProps(const Expression* expr, ExpressionProps& exprProps); - bool evaluableExpr(const Expression* expr) const; - static StatusOr checkPropNonexistOrDuplicate(const ColsDef& cols, folly::StringPiece prop, const std::string& validator); From a1255a32714d5fdc748945b28ee9a9c6c599ad5e Mon Sep 17 00:00:00 2001 From: jimingquan Date: Thu, 14 Oct 2021 20:01:28 +0800 Subject: [PATCH 4/5] fix pytest error --- tests/query/stateless/test_schema.py | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/tests/query/stateless/test_schema.py b/tests/query/stateless/test_schema.py index b0b27ca3851..0e13d8d228a 100644 --- a/tests/query/stateless/test_schema.py +++ b/tests/query/stateless/test_schema.py @@ -148,20 +148,6 @@ def test_alter_tag_succeed(self): ['age', 'int64', 'YES', T_EMPTY, T_EMPTY]] self.check_result(resp, expect) - # alter all - resp = self.execute('ALTER TAG student drop (name),' - 'ADD (gender string),' - 'CHANGE (gender int)') - self.check_resp_succeeded(resp) - - resp = self.execute('DESC TAG student') - self.check_resp_succeeded(resp) - expect = [['email', 'string', 'YES', T_EMPTY, T_EMPTY], - ['birthday', 'timestamp', 'YES', T_EMPTY, T_EMPTY], - ['age', 'int64', 'YES', T_EMPTY, T_EMPTY], - ['gender', 'int64', 'YES', T_EMPTY, T_EMPTY]] - self.check_result(resp, expect) - def test_alter_tag_failed(self): # alter ttl_col on wrong type try: @@ -304,19 +290,6 @@ def test_alter_edge_succeed(self): ['start_year', 'int64', 'YES', T_EMPTY, T_EMPTY]] self.check_result(resp, expect) - # alter all - resp = self.execute('ALTER EDGE relationship drop (name),' - 'ADD (end_year string),' - 'CHANGE (end_year int)') - self.check_resp_succeeded(resp) - - resp = self.execute('DESC EDGE relationship') - self.check_resp_succeeded(resp) - expect = [['email', 'string', 'YES', T_EMPTY, T_EMPTY], - ['start_year', 'int64', 'YES', T_EMPTY, T_EMPTY], - ['end_year', 'int64', 'YES', T_EMPTY, T_EMPTY]] - self.check_result(resp, expect) - def test_alter_edge_failed(self): # alter ttl_col on wrong type try: From 8b31823a28d09ef0f60eddbd1f8f9556f3379ecf Mon Sep 17 00:00:00 2001 From: jimingquan Date: Mon, 18 Oct 2021 10:26:23 +0800 Subject: [PATCH 5/5] address comment --- src/graph/context/ast/QueryAstContext.h | 4 ++-- src/graph/planner/ngql/MaintainPlanner.cpp | 8 ++++---- src/graph/validator/MaintainValidator.cpp | 17 +++++++---------- src/graph/validator/MaintainValidator.h | 8 ++++---- 4 files changed, 17 insertions(+), 20 deletions(-) diff --git a/src/graph/context/ast/QueryAstContext.h b/src/graph/context/ast/QueryAstContext.h index a1e09dab85c..b4fb5596fbc 100644 --- a/src/graph/context/ast/QueryAstContext.h +++ b/src/graph/context/ast/QueryAstContext.h @@ -157,12 +157,12 @@ struct FetchEdgesContext final : public AstContext { std::string inputVarName; }; -struct AlterContext final : public AstContext { +struct AlterSchemaContext final : public AstContext { std::vector schemaItems; meta::cpp2::SchemaProp schemaProps; }; -struct CreateContext final : public AstContext { +struct CreateSchemaContext final : public AstContext { bool ifNotExist{false}; std::string name; meta::cpp2::Schema schema; diff --git a/src/graph/planner/ngql/MaintainPlanner.cpp b/src/graph/planner/ngql/MaintainPlanner.cpp index 02d009e6432..2d3e92debe6 100644 --- a/src/graph/planner/ngql/MaintainPlanner.cpp +++ b/src/graph/planner/ngql/MaintainPlanner.cpp @@ -13,7 +13,7 @@ namespace nebula { namespace graph { StatusOr CreateTagPlanner::transform(AstContext* astCtx) { - auto createCtx = static_cast(astCtx); + auto createCtx = static_cast(astCtx); SubPlan plan; plan.root = plan.tail = CreateTag::make(createCtx->qctx, nullptr, @@ -24,7 +24,7 @@ StatusOr CreateTagPlanner::transform(AstContext* astCtx) { } StatusOr CreateEdgePlanner::transform(AstContext* astCtx) { - auto createCtx = static_cast(astCtx); + auto createCtx = static_cast(astCtx); SubPlan plan; plan.root = plan.tail = CreateEdge::make(createCtx->qctx, nullptr, @@ -35,7 +35,7 @@ StatusOr CreateEdgePlanner::transform(AstContext* astCtx) { } StatusOr AlterTagPlanner::transform(AstContext* astCtx) { - auto alterCtx = static_cast(astCtx); + auto alterCtx = static_cast(astCtx); auto qctx = alterCtx->qctx; auto name = *static_cast(alterCtx->sentence)->name(); SubPlan plan; @@ -49,7 +49,7 @@ StatusOr AlterTagPlanner::transform(AstContext* astCtx) { } StatusOr AlterEdgePlanner::transform(AstContext* astCtx) { - auto alterCtx = static_cast(astCtx); + auto alterCtx = static_cast(astCtx); auto qctx = alterCtx->qctx; auto name = *static_cast(alterCtx->sentence)->name(); SubPlan plan; diff --git a/src/graph/validator/MaintainValidator.cpp b/src/graph/validator/MaintainValidator.cpp index e463a2c01ba..a2bfeb595d1 100644 --- a/src/graph/validator/MaintainValidator.cpp +++ b/src/graph/validator/MaintainValidator.cpp @@ -70,10 +70,9 @@ static StatusOr> validateSchemaOpts( if (opType == meta::cpp2::AlterSchemaOp::DROP) { const auto &colNames = schemaOpt->columnNames(); for (auto &colName : colNames) { - if (uniqueColName.find(*colName) != uniqueColName.end()) { + if (!uniqueColName.emplace(*colName).second) { return Status::SemanticError("Duplicate column name `%s'", colName->c_str()); } - uniqueColName.emplace(*colName); meta::cpp2::ColumnDef column; column.name = *colName; schema.columns_ref().value().emplace_back(std::move(column)); @@ -81,10 +80,9 @@ static StatusOr> validateSchemaOpts( } else { const auto &specs = schemaOpt->columnSpecs(); for (auto &spec : specs) { - if (uniqueColName.find(*spec->name()) != uniqueColName.end()) { + if (!uniqueColName.emplace(*spec->name()).second) { return Status::SemanticError("Duplicate column name `%s'", spec->name()->c_str()); } - uniqueColName.emplace(*spec->name()); } NG_LOG_AND_RETURN_IF_ERROR(validateColumns(specs, schema)); } @@ -133,16 +131,15 @@ static Status checkColName(const std::vector specs) { std::unordered_set uniqueColName; for (const auto &spec : specs) { auto name = *spec->name(); - if (uniqueColName.find(name) != uniqueColName.end()) { + if (!uniqueColName.emplace(name).second) { return Status::SemanticError("Duplicate column name `%s'", name.c_str()); } - uniqueColName.emplace(name); } return Status::OK(); } Status CreateTagValidator::validateImpl() { - createCtx_ = getContext(); + createCtx_ = getContext(); auto sentence = static_cast(sentence_); createCtx_->ifNotExist = sentence->isIfNotExist(); auto name = *sentence->name(); @@ -165,7 +162,7 @@ Status CreateTagValidator::validateImpl() { } Status CreateEdgeValidator::validateImpl() { - createCtx_ = getContext(); + createCtx_ = getContext(); auto sentence = static_cast(sentence_); createCtx_->ifNotExist = sentence->isIfNotExist(); auto name = *sentence->name(); @@ -210,7 +207,7 @@ Status DescEdgeValidator::toPlan() { } Status AlterTagValidator::validateImpl() { - alterCtx_ = getContext(); + alterCtx_ = getContext(); auto sentence = static_cast(sentence_); auto schemaItems = validateSchemaOpts(sentence->getSchemaOpts()); NG_RETURN_IF_ERROR(schemaItems); @@ -222,7 +219,7 @@ Status AlterTagValidator::validateImpl() { } Status AlterEdgeValidator::validateImpl() { - alterCtx_ = getContext(); + alterCtx_ = getContext(); auto sentence = static_cast(sentence_); auto schemaItems = validateSchemaOpts(sentence->getSchemaOpts()); NG_RETURN_IF_ERROR(schemaItems); diff --git a/src/graph/validator/MaintainValidator.h b/src/graph/validator/MaintainValidator.h index c3d88fa8e2a..7b02a0a9fb9 100644 --- a/src/graph/validator/MaintainValidator.h +++ b/src/graph/validator/MaintainValidator.h @@ -24,7 +24,7 @@ class CreateTagValidator final : public Validator { private: Status validateImpl() override; - std::unique_ptr createCtx_; + std::unique_ptr createCtx_; }; class CreateEdgeValidator final : public Validator { @@ -36,7 +36,7 @@ class CreateEdgeValidator final : public Validator { private: Status validateImpl() override; - std::unique_ptr createCtx_; + std::unique_ptr createCtx_; }; class DescTagValidator final : public Validator { @@ -90,7 +90,7 @@ class AlterTagValidator final : public Validator { private: Status validateImpl() override; - std::unique_ptr alterCtx_; + std::unique_ptr alterCtx_; }; class AlterEdgeValidator final : public Validator { @@ -102,7 +102,7 @@ class AlterEdgeValidator final : public Validator { private: Status validateImpl() override; - std::unique_ptr alterCtx_; + std::unique_ptr alterCtx_; }; class ShowTagsValidator final : public Validator {