Ttl schema #422

ayyt · 2019-05-23T14:14:47Z

TTL feature in syntax and meta schema.
Contains the following content:

create tag/edge
alter tag/edge
show create tag/edge

describe tag/edge  only display column information, not display schema attributes.

show create tag/edge in PR #439

nebula-community-bot · 2019-05-23T14:15:04Z

Can one of the admins verify this patch?

ayyt · 2019-06-03T10:29:23Z

Refactor and rebase master, please review code, thx.

dangleptr · 2019-06-04T07:18:08Z

src/dataman/ResultSchemaProvider.cpp

@@ -46,7 +46,8 @@ bool ResultSchemaProvider::ResultSchemaField::isValid() const {
 *
 **********************************/
 ResultSchemaProvider::ResultSchemaProvider(Schema schema)
-        : columns_(std::move(schema.get_columns())) {
+        : columns_(std::move(schema.get_columns())),
+          schemaProp_(std::move(schema.get_schema_prop())) {


Result schema is used for Result. Please don't put schema props here.

@dangleptr , 👍👍
Yeah, in the initial design, desc tag/edge not only displays column information, but also shows schema attributes.

This way is now abandoned. The desc tag/edge only displays column information.

Show create tag/edge not only displays column information, but also shows the schema attributes.(PR #496 )

Today I am going to update this PR, thx.

@dangleptr , ResultSchemaProvider used in storage is used to read filter. So we need schema props in ResultSchemaProvider

Use NebulaSchemaProvider, ResultSchemaProvider is designed for result.

👍，Thank you very much for the guidance offline. I will fix it.

ayyt · 2019-06-05T10:40:59Z

Fix and rebase master. please code review again, thx

ayyt · 2019-06-10T10:42:05Z

Fix conflicts and rebase master. please code review again.

laura-ding · 2019-06-11T07:04:57Z

src/graph/test/SchemaTest.cpp

+        auto code = client->execute(query, resp);
+        ASSERT_EQ(cpp2::ErrorCode::SUCCEEDED, code);
+    }
+    sleep(FLAGS_load_data_interval_secs + 1);


Delete all sleep after rebase master

Good catch. 👍
I will strengthen the review work in the future, and I apologize here.

laura-ding · 2019-06-11T07:23:31Z

src/interface/common.thrift

@@ -66,8 +66,14 @@ struct ColumnDef {
    2: required ValueType type,
 }

+struct SchemaProp {


CREATE TAG person(name string) ttl_duration = 100, ttl_col = name
Here, the name call option, ttl_duration call property，but in INSERT VERTEX (name) the name call property. Please call it the same!

Good point.

CREATE TAG person(name string) ttl_duration = 100, ttl_col = name
in parser.yy like
KW_CREATE KW_TAG name_label L_PAREN column_spec_list R_PAREN create_schema_prop_list
in meta.thrift like

struct CreateTagReq { 1: common.GraphSpaceID space_id, 2: string tag_name, 3: common.Schema schema, } struct Schema { 1: list<ColumnDef> columns, 2: SchemaProp schema_prop, } struct ColumnDef { 1: required string name, 2: required ValueType type, } struct SchemaProp { 1: i64 ttl_duration, 2: string ttl_col, }

Here, the name is a ColumnDef in Schema.columns, ttl_duration is in SchemaProp

INSERT VERTEX (name)

in parser.yy like

insert_vertex_sentence : KW_INSERT KW_VERTEX vertex_tag_list KW_VALUES vertex_row_list vertex_tag_item : name_label L_PAREN prop_list R_PAREN

in storage.thrift

struct AddVerticesRequest { 1: common.GraphSpaceID space_id, // partId => vertices 2: map<common.PartitionID, list<Vertex>>(cpp.template = "std::unordered_map") parts, // If true, it equals an upsert operation. 3: bool overwritable, } struct Vertex { 1: common.VertexID id, 2: list<Tag> tags, } struct Tag { 1: common.TagID tag_id, 2: binary props, }

Here props in Tag struct contains schema and the column value to insert.

so name in INSERT VERTEX (name) is in schema of props and is also a ColumnDef in Schema.columns

By the way, in nGQL

ALTER TAG woman Drop (name, email) ttl_duration = 200

In meta.thrift like:

struct AlterTagReq { 1: common.GraphSpaceID space_id, 2: string tag_name, 3: list<AlterSchemaOption> schema_options, 4: list<AlterSchemaProp> schema_props, } struct AlterSchemaOption { 1: AlterSchemaOptionType type, 2: common.Schema schema, }

Drop (name, email) is called a AlterSchemaOption， but name is also a ColumnDef in AlterSchemaOption.schema.columns

laura-ding · 2019-06-11T08:01:29Z

src/meta/MetaServiceUtils.cpp

+    }
+}
+
+Status MetaServiceUtils::checkSchemaTTLProp(nebula::cpp2::SchemaProp& prop) {


I think it makes more sense to put this check on graphd

Good, 👍
I will fixed

I carefully thought about it.
Here checkSchemaTTLProp uses alter tag/edge sentence.
checkSchemaTTLProp can be executed after the schema of the alter statement is updated in meta.

ayyt · 2019-06-12T06:56:46Z

Fix laura-ding's comments and rebase master, please review again.

dangleptr

Well done~ The PR LGTM now. Thanks for your effort! @steppenwolfyuetong

whitewum · 2019-06-27T06:57:18Z

@steppenwolfyuetong please modify nGQL.md to address TTL usage. Or at least give explanations at the beginning of this pr, and we can help to improve nGQL.md
And please add more tests in the graph layer.

ayyt · 2019-06-27T07:24:03Z

@steppenwolfyuetong please modify nGQL.md to address TTL usage. Or at least give explainations at the beginning of this pr, and we can help to improve nGQL.md

Yeah， This Pr is just the second part of the ttl feature.
I want to update the nGQL.md when the TTL feature is completed, that is, when the Ttl read filter #478 PR is submitted. WDYT？

Please accept it first. thx

whitewum · 2019-06-27T07:36:55Z

src/graph/SchemaHelper.h

@@ -0,0 +1,44 @@
+/* Copyright (c) 2019 vesoft inc. All rights reserved.


any tests to cover this class and all the public functions ?

create tag/edge and alter tag/edge use these public functions.

whitewum · 2019-06-27T07:43:13Z

src/graph/SchemaHelper.cpp

+
+
+// static
+Status SchemaHelper::setTTLDuration(SchemaPropItem* schemaProp, nebula::cpp2::Schema& schema) {


prefer to const schemaProp if no modifications.

The original pointer type here is non-const

whitewum · 2019-06-27T07:45:35Z

src/graph/SchemaHelper.cpp

+    for (auto& col : schema.columns) {
+        if (col.name == ttlColName) {
+            // Only integer columns and timestamp columns can be used as ttl_col
+            if (col.type.type != nebula::cpp2::SupportedType::INT &&


why only int & timestamp?
why not double and datetime? datetime is common. at least add a TODO to mark this.

Types that can be converted to 64-bit integers should be.
double type can not be.
If the datetime type is stored according to the timestamp, here I add a TODO

laura-ding

LGTM

whitewum · 2019-06-27T08:36:42Z

src/graph/test/SchemaTest.cpp

+        std::string query = "DESCRIBE TAG person";
+        auto code = client->execute(query, resp);
+        ASSERT_EQ(cpp2::ErrorCode::SUCCEEDED, code);
+        std::vector<uniform_tuple_t<std::string, 2>> expected{


this expected result looks duplicated (to check unchanged). U can make it outside to be seen hereafter. maybe call it another name, e.g., rawExpected.

tag/edge schema properies use show create tag/edge to display, not use desc tag/edge.
Here I add a TODO
I will improve it in PR ” Support show create tag/edge xxx, show create space xxx SQL ” #496

whitewum · 2019-06-27T08:42:50Z

src/graph/test/SchemaTest.cpp

+        cpp2::ExecutionResponse resp;
+        std::string query = "CREATE EDGE woman(name string, email string, "
+                            "age int, gender string, row_timestamp timestamp)"
+                            "ttl_duration = -100, ttl_col = row_timestamp";


try to test ttl_duration = 0 // what do u want if ttl == 0?

Yeah, ttl_duration default value is 0.
< 0 represens infinity

whitewum · 2019-06-27T08:45:23Z

src/parser/parser.yy

@@ -537,6 +537,9 @@ create_schema_prop_list

 create_schema_prop_item
    : KW_TTL_DURATION ASSIGN INTEGER {
+        if ($3 < 0) {


what's your desgin for ttl == 0?

< = 0 represents infinity.
so <0 to =0
Here I add a comment.

whitewum · 2019-06-27T08:54:10Z

src/meta/MetaServiceUtils.cpp

+                        cols.erase(it);
+                        return cpp2::ErrorCode::SUCCEEDED;
+                    } else {
+                        LOG(WARNING) << "Column cant't be dropped, a TTL attribute on it : "


👍👍👍

whitewum

I'm generally OK with this pr, with a few questions.

whitewum · 2019-06-27T09:18:05Z

src/meta/MetaServiceUtils.cpp

+                if (colName == it->get_name()) {
+                    // Check if there is a TTL on the column to be deleted
+                    if (!prop.get_ttl_col() ||
+                        (prop.get_ttl_col() && (*prop.get_ttl_col() != colName))) {


i'm a bit curious about *prop.get_ttl_col() != colName

What do you want to know

whitewum · 2019-06-27T09:20:31Z

src/meta/MetaServiceUtils.cpp

+    }
+
+    // Disable implicit TTL mode
+    if ((schemaProp.get_ttl_duration() && (*schemaProp.get_ttl_duration() !=0)) &&


ayyt · 2019-06-27T10:04:23Z

jenkins go

nebula-community-bot · 2019-06-27T10:18:22Z

Unit testing passed.

nebula-community-bot · 2019-06-27T10:23:37Z

Unit testing passed.

ayyt · 2019-06-27T10:24:42Z

Fix comments

whitewum

GREAT

laura-ding

LGTM

nebula-community-bot · 2019-06-28T07:34:32Z

Unit testing passed.

* support time to live alter tag/edge drop column, and the column to be delete is as TTL column * improve unreserved keywords feature * address laura-ding's comment * address dangleptr's comment * address dangleptr's comment * In AlterTagReq/AlterEdgeReq discards AlterSchemaProp, using SchemaProp * address comments * address dangleptr's comments * address comments

* add insert vertex only parser add insert vertex only validator fix grammar adjust insert vertex parser * add test * format * merge master * modify parser * add fetch test * format * fix fetch vertex without tag error Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> Co-authored-by: hs.zhang <22708345+cangfengzhs@users.noreply.github.com> Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>

ayyt force-pushed the TTL_schema branch 3 times, most recently from 66386fc to 7e14da3 Compare May 27, 2019 07:56

ayyt force-pushed the TTL_schema branch 5 times, most recently from 87669de to 9ecfda7 Compare June 3, 2019 08:29

ayyt force-pushed the TTL_schema branch from 9f26e07 to ef96547 Compare June 4, 2019 06:10

dangleptr reviewed Jun 4, 2019

View reviewed changes

ayyt mentioned this pull request Jun 4, 2019

Support show create tag/edge xxx, show create space xxx SQL #496

Merged

ayyt force-pushed the TTL_schema branch 2 times, most recently from 31d9578 to 2cde598 Compare June 5, 2019 10:40

ayyt force-pushed the TTL_schema branch 6 times, most recently from b9174fe to c0d7504 Compare June 10, 2019 10:41

ayyt added the ready-for-testing PR: ready for the CI test label Jun 10, 2019

ayyt force-pushed the TTL_schema branch from c0d7504 to ce7d229 Compare June 11, 2019 02:28

laura-ding reviewed Jun 11, 2019

View reviewed changes

ayyt force-pushed the TTL_schema branch 3 times, most recently from feaf147 to 4f687cf Compare June 12, 2019 06:54

dangleptr previously approved these changes Jun 27, 2019

View reviewed changes

whitewum reviewed Jun 27, 2019

View reviewed changes

laura-ding previously approved these changes Jun 27, 2019

View reviewed changes

whitewum reviewed Jun 27, 2019

View reviewed changes

whitewum previously approved these changes Jun 27, 2019

View reviewed changes

address comments

e69f80c

ayyt dismissed stale reviews from whitewum, laura-ding, and dangleptr via e69f80c June 27, 2019 09:52

whitewum reviewed Jun 28, 2019

View reviewed changes

whitewum approved these changes Jun 28, 2019

View reviewed changes

laura-ding approved these changes Jun 28, 2019

View reviewed changes

Merge branch 'master' into TTL_schema

d2d2b53

dangleptr merged commit 51e81a7 into vesoft-inc:master Jun 28, 2019

ayyt deleted the TTL_schema branch June 28, 2019 07:42

amber-moe self-assigned this Feb 17, 2020

panda-sheep mentioned this pull request Feb 26, 2020

Support Time-to-live (TTL) #163

Closed

		@@ -0,0 +1,44 @@
		/* Copyright (c) 2019 vesoft inc. All rights reserved.



		// static
		Status SchemaHelper::setTTLDuration(SchemaPropItem* schemaProp, nebula::cpp2::Schema& schema) {

Ttl schema #422

Ttl schema #422

Conversation

ayyt commented May 23, 2019 • edited Loading

nebula-community-bot commented May 23, 2019

ayyt commented Jun 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayyt commented Jun 5, 2019 • edited Loading

ayyt commented Jun 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayyt Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayyt commented Jun 12, 2019

dangleptr left a comment

Choose a reason for hiding this comment

whitewum commented Jun 27, 2019 • edited Loading

ayyt commented Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laura-ding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayyt Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitewum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayyt commented Jun 27, 2019

nebula-community-bot commented Jun 27, 2019

nebula-community-bot commented Jun 27, 2019

ayyt commented Jun 27, 2019

whitewum left a comment

Choose a reason for hiding this comment

laura-ding left a comment

Choose a reason for hiding this comment

nebula-community-bot commented Jun 28, 2019

ayyt commented May 23, 2019 •

edited

Loading

ayyt commented Jun 5, 2019 •

edited

Loading

ayyt Jun 11, 2019 •

edited

Loading

whitewum commented Jun 27, 2019 •

edited

Loading

ayyt commented Jun 27, 2019 •

edited

Loading

ayyt Jun 27, 2019 •

edited

Loading