Skip to content

vschema alter syntax#4412

Merged
sougou merged 3 commits intovitessio:masterfrom
tinyspeck:vschema-alter-syntax
Dec 8, 2018
Merged

vschema alter syntax#4412
sougou merged 3 commits intovitessio:masterfrom
tinyspeck:vschema-alter-syntax

Conversation

@demmer
Copy link
Copy Markdown
Member

@demmer demmer commented Dec 3, 2018

Description

Rework the vschema DDL syntax to be clearer and more consistent and add the ability to manage unsharded keyspaces.

NOTE: This was rebased on top of #4385 now that it's in master.

Details

This PR reworks the syntax originally defined in #3459 with the benefit of hindsight and to add a missing feature to manage the vschema for unsharded keyspaces.

The new syntax implemented in this PR is:

Create / remove vindexes (note that for drop vindex the syntax is supported but the executor is not yet updated):

alter vschema create vindex hash using hash
alter vschema drop vindex hash

Associate vindexes with tables:

alter vschema on t add vindex hash(id)
alter vschema on t drop vindex hash

Associate vindex and define in one step:

alter vschema on t add vindex hash(id) using hash
alter vschema on t add vindex t_lookup (c1,c2) using lookup with owner=`test`, from=`c1,c2`, table=test_lookup, to=keyspace_id"

New syntax for adding / removing tables from unsharded keyspaces:

alter vschema add table test_table
alter vschema drop table test_table

New show statements:

show vschema vindexes
show vschema tables

@demmer demmer requested a review from sougou as a code owner December 3, 2018 18:47
@demmer demmer force-pushed the vschema-alter-syntax branch from 46a054e to fed1607 Compare December 4, 2018 18:28
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why this change? Not obvious to me why we are adding this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we remove this token now from the grammar? It does not look like we need VSCHEMA_TABLES anymore

@rafael
Copy link
Copy Markdown
Member

rafael commented Dec 5, 2018

Demmer I reviewed these three commits in the context of this PR:

  • dbde27a
  • 49842de
  • fed1607

Looks good to me. Added a few comments. Let me know your thoughts.

Rework the vschema syntax so that instead of top-level create/drop
statements and alter table we use a consistent syntax of:

alter vschema create vindex...
alter vschema drop vindex ...
alter vschema on <table> add vindex ...
alter vschema on <table> drop vindex ...

Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Make the show syntax consistent:
`show vschema vindexes`
`show vschema tables`

Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Since unsharded keyspaces don't have vindex definitions, implement
a new syntax for adding / removing tables from the vschema:

`alter vschema add table test_table`
`alter vschema drop table test_table`

Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
@demmer demmer force-pushed the vschema-alter-syntax branch from fed1607 to b55d155 Compare December 7, 2018 17:41
@demmer
Copy link
Copy Markdown
Member Author

demmer commented Dec 8, 2018

@sougou Since this changes the alter syntax I think we should include it in 3.0 so we can then document this officially as the better way to do vschema operations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants