Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize the ttl grammar for label the ttl_col , alter grammer. #1874

Closed
wants to merge 3 commits into from
Closed

Optimize the ttl grammar for label the ttl_col , alter grammer. #1874

wants to merge 3 commits into from

Conversation

Shylock-Hg
Copy link
Contributor

@Shylock-Hg Shylock-Hg commented Mar 5, 2020

What changes were proposed in this pull request?

Change the grammar about ttl, not change the inner implement just modify the parser.

  1. Change ttl_col from string to label typed.
    such as
    from
create tag tag1(id int, name string) ttl_col = "id", ttl_duration = 1

to

create tag tag1(id int, name string) ttl_col = id, ttl_duration = 1
  1. Alter grammar
    drop from
alter tag tag1 ttl_col = ""

to

alter tag tag1 drop ttl

change from

alter tag tag1 ttl_col = 'id', ttl_duration = 444

to

alter tag tag1 change ttl_col = id, ttl_duration = 444

Why are the changes needed?

  1. Avoid using string in label place to reduce user confuse
  2. Avoid assign statement represent drop semantic

Does this PR introduce any user-facing change?

Yes. @Amber1990Zhang

How was this patch tested?

@Shylock-Hg Shylock-Hg added ready-for-testing PR: ready for the CI test nGQL labels Mar 5, 2020
@Shylock-Hg Shylock-Hg linked an issue Mar 5, 2020 that may be closed by this pull request
@Shylock-Hg Shylock-Hg added the incompatible PR: incompatible with the recently released version label Mar 6, 2020
@panda-sheep
Copy link
Contributor

Good job, but I don't feel very friendly to user
alter tag t drop ttl; will make the user feel that this is deleting a column, the column name is ttl

@Shylock-Hg
Copy link
Contributor Author

Good job, but I don't feel very friendly to user
alter tag t drop ttl; will make the user feel that this is deleting a column, the column name is ttl

Delete the column is different.

Copy link
Contributor

@laura-ding laura-ding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ttl_col is the table properpty, I prefer to stay the same or add a new keyword.
such as SET.

GQLParser parser;
std::string query = "ALTER TAG man ttl_duration = 200";
std::string query = "ALTER TAG person "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessary to add the keyword KW_TTL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but drop ttl_col sounds like drop the schema field.

@Shylock-Hg
Copy link
Contributor Author

The ttl_col is the table properpty, I prefer to stay the same or add a new keyword.
such as SET.

Such as alter tag tag1 set ttl_col = label?

@laura-ding
Copy link
Contributor

The ttl_col is the table properpty, I prefer to stay the same or add a new keyword.
such as SET.

Such as alter tag tag1 set ttl_col = label?

Yes!

@amber-moe amber-moe changed the title Optimize the ttl grammer for label the ttl_col , alter grammer. Optimize the ttl grammar for label the ttl_col , alter grammer. Mar 11, 2020
@amber-moe amber-moe self-assigned this Mar 11, 2020
Copy link
Contributor

@dangleptr dangleptr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think current changes is good. We could take mysql "alter table" as our reference.

@Shylock-Hg
Copy link
Contributor Author

A better way in #1922

@Shylock-Hg Shylock-Hg closed this Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incompatible PR: incompatible with the recently released version ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change the ttl_col to label typed instead of string
5 participants