Support TTL for index #382

bright-starry-sky · 2021-03-10T09:40:33Z

Not ready to review

bright-starry-sky · 2021-03-23T07:12:48Z

TODO :

test cases for storage side.
correct test cases for graph side.

bright-starry-sky · 2021-03-24T13:15:00Z

Added test cases for storage side.

bright-starry-sky · 2021-03-25T07:44:02Z

test cases changed for graph side :
vesoft-inc/nebula-graph#876

bright-starry-sky · 2021-03-25T07:44:23Z

Ready to review.

CLAassistant · 2021-03-26T01:49:06Z

All committers have signed the CLA.

critical27

LGTM, good job. Any related PR?

bright-starry-sky · 2021-03-29T03:43:30Z

LGTM, good job. Any related PR?

After this one merged , test case need to correct for graph side : vesoft-inc/nebula-graph#876

critical27 · 2021-03-29T08:39:30Z

src/storage/CommonUtils.cpp

+
+StatusOr<Value> CommonUtils::ttlValue(const meta::SchemaProviderIf* schema, RowReader* reader) {
+ DCHECK(schema != nullptr);
+ const auto* ns = dynamic_cast<const meta::NebulaSchemaProvider*>(schema);


We could remove SchemaProviderIf later if necessary, it is useless now.

We could remove SchemaProviderIf later if necessary, it is useless now.

Yes, I see other place that need to be changed, Let me change them later.

darionyaphet · 2021-03-30T02:10:10Z

src/storage/admin/RebuildTagIndexTask.cpp

+ continue;
+ }
+
+ auto ttlProp = CommonUtils::ttlProps(schema.get());


Could move to rebuild index task ？

Could move to rebuild index task ？

What's the purpose?

bright-starry-sky · 2021-04-01T04:28:16Z

Fixed compile error and rebased master.

panda-sheep · 2021-04-02T08:49:57Z

src/meta/processors/schemaMan/AlterTagProcessor.cpp

+ if (prop.get_ttl_col()) {
+ col = *prop.get_ttl_col();
+ }
+ if (!col.empty() && duration > 0) {


Can ttl and index coexist?

Aha, they can coexist. I saved the timestamp value to index val.

existIndex means there is an index,
!col.empty() && duration> 0 means there is ttl

Why is there an error here?

existIndex means there is an index,
!col.empty() && duration> 0 means there is ttl

Why is there an error here?

if index exists, not allow to alter TTL properties.

yeah, i know what you mean.
But the prop schema prop is in the schema, not the schema prop in AlterTagReq.

Your current logic is that when alter tag, as long as there is index and ttl, an error will be reported.

yeah, i know what you mean.
But the prop schema prop is in the schema, not the schema prop in AlterTagReq.

Your current logic is that when alter tag, as long as there is index and ttl, an error will be reported.

yes，there should be an error thrown here, either alter ttl or alter column (excepted for add column )

I mean:
here logic is unreasonable.
As long as there are index and ttl, here are not allowed to alter tag

I mean:
here logic is unreasonable.
As long as there are index and ttl, here are not allowed to alter tag

good point. 🐂B

panda-sheep

Good job.
could you add a UT that a tag has already a index and ttl, and then alter this tag（not alter ttl prop）？

bright-starry-sky · 2021-04-12T02:30:40Z

Good job.
could you add a UT that a tag has already a index and ttl, and then alter this tag（not alter ttl prop）？

Aha, the old test cases are still valid for storage side. for graph side, the below PR to do with what you said.

bright-starry-sky · 2021-04-12T02:31:27Z

Good job.
could you add a UT that a tag has already a index and ttl, and then alter this tag（not alter ttl prop）？

Aha, the old test cases are still valid for storage side. for graph side, the below PR to do with what you said.

vesoft-inc/nebula-graph#876

bright-starry-sky · 2021-04-12T06:06:46Z

Addressed panda-sheep's comments.

panda-sheep

Good job！

critical27

Good job!

bright-starry-sky · 2021-04-12T07:44:45Z

This file src/meta/test/ProcessorTest.cpp is too large and often causes the link to fail on CI system (ubuntu1804, gcc-9.2).
I'm going to split it in the next PR.

bright-starry-sky added the wip Solution: work in progress label Mar 10, 2021

bright-starry-sky force-pushed the index_ttl branch 4 times, most recently from 73c9bb8 to 12de655 Compare March 23, 2021 07:05

bright-starry-sky added ready-for-testing PR: ready for the CI test doc affected Solution: improvements or additions to documentation and removed wip Solution: work in progress labels Mar 23, 2021

bright-starry-sky force-pushed the index_ttl branch from 12de655 to 30cb073 Compare March 24, 2021 13:05

bright-starry-sky requested review from panda-sheep, critical27, darionyaphet and liuyu85cn March 25, 2021 05:06

bright-starry-sky added the Ready-to-review label Mar 25, 2021

bright-starry-sky mentioned this pull request Mar 25, 2021

improved testcases for index ttl vesoft-inc/nebula-graph#876

Merged

critical27 reviewed Mar 29, 2021

View reviewed changes

critical27 previously approved these changes Mar 29, 2021

View reviewed changes

darionyaphet reviewed Mar 30, 2021

View reviewed changes

bright-starry-sky dismissed critical27’s stale review via d698e52 April 1, 2021 04:27

bright-starry-sky force-pushed the index_ttl branch from 837af26 to d698e52 Compare April 1, 2021 04:27

critical27 previously approved these changes Apr 2, 2021

View reviewed changes

panda-sheep reviewed Apr 2, 2021

View reviewed changes

panda-sheep reviewed Apr 8, 2021

View reviewed changes

bright-starry-sky added 6 commits April 12, 2021 14:05

support ttl for index

cb161e9

meta related

55489c0

add test cases

764e102

ci test

d563fc1

fixed compile error and rebased master

fbf0c94

Addressed panda's comments

4b28c7e

bright-starry-sky dismissed critical27’s stale review via 4b28c7e April 12, 2021 06:06

bright-starry-sky force-pushed the index_ttl branch from 25cd467 to 4b28c7e Compare April 12, 2021 06:06

Merge branch 'master' into index_ttl

f87dd7c

panda-sheep approved these changes Apr 12, 2021

View reviewed changes

critical27 approved these changes Apr 12, 2021

View reviewed changes

bright-starry-sky merged commit b2851b4 into vesoft-inc:master Apr 12, 2021

bright-starry-sky deleted the index_ttl branch April 12, 2021 07:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support TTL for index #382

Support TTL for index #382

bright-starry-sky commented Mar 10, 2021

bright-starry-sky commented Mar 23, 2021

bright-starry-sky commented Mar 24, 2021

bright-starry-sky commented Mar 25, 2021

bright-starry-sky commented Mar 25, 2021

CLAassistant commented Mar 26, 2021 •

edited

Loading

critical27 left a comment •

edited

Loading

bright-starry-sky commented Mar 29, 2021 •

edited

Loading

critical27 Mar 29, 2021

bright-starry-sky Mar 30, 2021

darionyaphet Mar 30, 2021

bright-starry-sky Mar 30, 2021

bright-starry-sky commented Apr 1, 2021

panda-sheep Apr 2, 2021

bright-starry-sky Apr 6, 2021 •

edited

Loading

panda-sheep Apr 6, 2021

bright-starry-sky Apr 6, 2021

panda-sheep Apr 6, 2021

bright-starry-sky Apr 8, 2021

panda-sheep Apr 12, 2021

bright-starry-sky Apr 12, 2021

panda-sheep left a comment

bright-starry-sky commented Apr 12, 2021

bright-starry-sky commented Apr 12, 2021

bright-starry-sky commented Apr 12, 2021

panda-sheep left a comment

critical27 left a comment

bright-starry-sky commented Apr 12, 2021

Support TTL for index #382

Support TTL for index #382

Conversation

bright-starry-sky commented Mar 10, 2021

bright-starry-sky commented Mar 23, 2021

bright-starry-sky commented Mar 24, 2021

bright-starry-sky commented Mar 25, 2021

bright-starry-sky commented Mar 25, 2021

CLAassistant commented Mar 26, 2021 • edited Loading

critical27 left a comment • edited Loading

Choose a reason for hiding this comment

bright-starry-sky commented Mar 29, 2021 • 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

bright-starry-sky commented Apr 1, 2021

Choose a reason for hiding this comment

bright-starry-sky Apr 6, 2021 • 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

panda-sheep left a comment

Choose a reason for hiding this comment

bright-starry-sky commented Apr 12, 2021

bright-starry-sky commented Apr 12, 2021

bright-starry-sky commented Apr 12, 2021

panda-sheep left a comment

Choose a reason for hiding this comment

critical27 left a comment

Choose a reason for hiding this comment

bright-starry-sky commented Apr 12, 2021

CLAassistant commented Mar 26, 2021 •

edited

Loading

critical27 left a comment •

edited

Loading

bright-starry-sky commented Mar 29, 2021 •

edited

Loading

bright-starry-sky Apr 6, 2021 •

edited

Loading