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

sql: allow manipulating unqualified index names. #10091

Merged
merged 1 commit into from
Oct 25, 2016

Conversation

knz
Copy link
Contributor

@knz knz commented Oct 19, 2016

With this patch, one can use a simple index name "idxname" instead of
"tablename@idxname" with DROP INDEX and ALTER INDEX.

Fixes #8800.

cc @dt


This change is Reviewable

@knz knz force-pushed the drop-index-search branch 2 times, most recently from 36d5126 to b18b5e7 Compare October 19, 2016 19:21
@maddyblue
Copy link
Contributor

Review status: 0 of 11 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


pkg/sql/split.go, line 53 at r1 (raw file):

  }

  if n.Index != nil && n.Index.SearchTable {

Why do we need the check for n.Index != nil here?

Also this if block is repeated almost verbatim (except for this n.Index check) in three places. Can we make a method on n to correctly set tn and n.Index?


pkg/sql/parser/sql.y, line 4391 at r1 (raw file):

    $$.val = &TableNameWithIndex{Table: $1.normalizableTableName(), Index: Name($3)}
  }
| qualified_name

Why not name?


Comments from Reviewable

@knz
Copy link
Contributor Author

knz commented Oct 21, 2016

Review status: 0 of 11 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


pkg/sql/split.go, line 53 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Why do we need the check for n.Index != nil here?

Also this if block is repeated almost verbatim (except for this n.Index check) in three places. Can we make a method on n to correctly set tn and n.Index?

The check for n.Index is because this node is used for both ALTER TABLE ... SPLIT AT and ALTER INDEX ... SPLIT AT.

But I refactored the code as suggested.


pkg/sql/parser/sql.y, line 4391 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Why not name?

Because if I don't have a current database in the planner I still want to do e.g. "DROP INDEX mydb.myindex".

Comments from Reviewable

@maddyblue
Copy link
Contributor

:lgtm:


Review status: 0 of 11 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


Comments from Reviewable

With this patch, one can use a simple index name "idxname" instead of
"tablename@idxname" with DROP INDEX and ALTER INDEX.
@knz
Copy link
Contributor Author

knz commented Oct 25, 2016

TFYR!


Review status: 0 of 11 files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@knz knz added the docs-todo label Oct 25, 2016
@knz knz merged commit 339bc74 into cockroachdb:master Oct 25, 2016
@knz knz deleted the drop-index-search branch October 25, 2016 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants