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: Adding support for show indexes from database command #37942

Merged
merged 1 commit into from Jun 3, 2019

Conversation

Projects
None yet
4 participants
@rohany
Copy link
Contributor

commented May 30, 2019

As requested in #37270, support for a show indexes from database command would be helpful. This PR includes support for the command in the parser. Future PR's will implement the functionality of the parsed results.

@rohany rohany requested review from solongordon and awoods187 May 30, 2019

@rohany rohany requested a review from cockroachdb/sql-language-prs as a code owner May 30, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented May 30, 2019

This change is Reviewable

@rohany

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

I updated the command to be SHOW DBINDEXES, as opposed to SHOW INDEXES FROM DATABASE <database>, due to some parsing issues for error messages.

@solongordon
Copy link
Contributor

left a comment

Hm, to be honest I don't love SHOW DBINDEXES, I think because it implies that a DBINDEX is something different from an INDEX. If SHOW INDEXES FROM DATABASE is definitely unworkable, maybe we could go with SHOW DATABASE INDEXES or SHOW ALL INDEXES? @knz might have an opinion about this.

Also we probably want to support the same aliases that SHOW INDEXES does, namely SHOW INDEX and SHOW KEYS. See https://www.cockroachlabs.com/docs/stable/show-index.html.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @awoods187 and @solongordon)

@rohany

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

I like show all indexes, i think that makes alot of sense.

@knz

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

I don't understand what problem you are encountering. The following patch works fine for me:

modified   pkg/sql/parser/sql.y
@@ -3347,18 +3347,26 @@ show_grants_stmt:

 // %Help: SHOW INDEXES - list indexes
 // %Category: DDL
-// %Text: SHOW INDEXES FROM <tablename>
+// %Text: SHOW INDEXES FROM { <tablename> | DATABASE <databasename> }
 // %SeeAlso: WEBDOCS/show-index.html
 show_indexes_stmt:
   SHOW INDEX FROM table_name
   {
     $$.val = &tree.ShowIndexes{Table: $4.unresolvedObjectName()}
   }
+| SHOW INDEX FROM DATABASE database_name
+  {
+    $$.val = &tree.ShowIndexes{...}
+  }
 | SHOW INDEX error // SHOW HELP: SHOW INDEXES
 | SHOW INDEXES FROM table_name
   {
     $$.val = &tree.ShowIndexes{Table: $4.unresolvedObjectName()}
   }
+| SHOW INDEXES FROM DATABASE database_name
+  {
+    $$.val = &tree.ShowIndexes{...}
+  }
 | SHOW INDEXES error // SHOW HELP: SHOW INDEXES
 | SHOW KEYS FROM table_name
   {

What's the problem?

(I also think that SHOW INDEXES without a FROM should be equivalent to SHOW INDEXES FROM DATABASE applied to the current database.)

@rohany

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

I was trying to display a different help message for the show indexes from database command. If I did the above and try and show a different message then I ran into issues differentiating the two.

@knz

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Keep the single error message, using the diff above -- this will be sufficient.

@rohany rohany force-pushed the rohany:index_show branch 2 times, most recently from 9e0d3c4 to 0696876 Jun 3, 2019

@knz
Copy link
Member

left a comment

Good progress. Next step is to update the parser tests.

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @awoods187)

@rohany rohany force-pushed the rohany:index_show branch from 0696876 to ba3800f Jun 3, 2019

@rohany rohany requested a review from cockroachdb/sql-rest-prs as a code owner Jun 3, 2019

@rohany

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

I'm just pulling in changes from the backend implementation as well, this way I can include the tests there. Overall its a small change, so no point in splitting it up into multiple PRs.

@rohany rohany changed the title sql: Adding support for show indexes from database command in the parser sql: Adding support for show indexes from database command Jun 3, 2019

@knz
Copy link
Member

left a comment

Good progress. Next step is to properly handle more complex database names.

Reviewed 8 of 8 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @awoods187 and @rohany)


pkg/sql/delegate/show_table.go, line 48 at r2 (raw file):

           implicit::BOOL
    FROM %s.information_schema.statistics`
	return parse(fmt.Sprintf(getAllIndexesQuery, n.Database))

This Sprintf is suspicious. Please consider how showTableDetails injects the database name and verify the same is done here.

Also please add a test with a database name containing special characters.


pkg/sql/logictest/testdata/logic_test/show_source, line 350 at r2 (raw file):

----
table1 primary false 1 key1 ASC false false
table2 primary false 1 key2 ASC false false

nit: missing newline at end of file


pkg/sql/sem/tree/show.go, line 159 at r2 (raw file):

// ShowDbIndexes represents a SHOW INDEXES FROM DATABASE statement.
type ShowDbIndexes struct {

style: the word is not so long that an abbreviation is warranted here. You can spell ShowDatabaseIndexes in full.


pkg/sql/sem/tree/show.go, line 160 at r2 (raw file):

// ShowDbIndexes represents a SHOW INDEXES FROM DATABASE statement.
type ShowDbIndexes struct {
	Database string

This is not a string, it should be a Name. You can imitate DropDatabase.


pkg/sql/sem/tree/show.go, line 166 at r2 (raw file):

func (node *ShowDbIndexes) Format(ctx *FmtCtx) {
	ctx.WriteString("SHOW INDEXES FROM DATABASE ")
	ctx.WriteString(node.Database)

ctx.FormatNode; again imitate DropDatabase.


pkg/sql/sem/tree/stmt.go, line 697 at r2 (raw file):

// StatementTag returns a short string identifying the type of statement.
func (*ShowIndexes) StatementTag() string { return "SHOW INDEXES" }

maybe change this to add FROM TABLE

@rohany

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@knz I addressed your comments. Regarding the Sprintf, the way I have done it is how showTableDetails substitutes in the database name as well.

@knz

knz approved these changes Jun 3, 2019

Copy link
Member

left a comment

LGTM. You may want to configure your editor to apply crlfmt automatically upon saving a file.

Reviewed 7 of 7 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @awoods187 and @rohany)


pkg/sql/delegate/show_table.go, line 48 at r2 (raw file):

Previously, knz (kena) wrote…

This Sprintf is suspicious. Please consider how showTableDetails injects the database name and verify the same is done here.

Also please add a test with a database name containing special characters.

Yes this works now because Database has become a Name. IF you had kept it a string, then the Sprintf would be inappropriate as-is.


pkg/sql/parser/parse_test.go, line 414 at r3 (raw file):

		{`SHOW INDEXES FROM a.b.c`},
		{`SHOW INDEXES FROM DATABASE a`},
		{`SHOW INDEXES FROM DATABASE "$speci@ltest"`},

nit: this is OK but we typically don't test special characters throughout the parser tests

sql: Implement support for a show indexes from database command
Implements the feature requested in #37270.

Release note (sql change): Support a show indexes from database command

@rohany rohany force-pushed the rohany:index_show branch from cdb6b6d to eba3b37 Jun 3, 2019

@rohany

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

bors r+

craig bot pushed a commit that referenced this pull request Jun 3, 2019

Merge #37942 #37977
37942: sql: Adding support for show indexes from database command r=rohany a=rohany

As requested in #37270, support for a show indexes from database command would be helpful. This PR includes support for the command in the parser. Future PR's will implement the functionality of the parsed results.

37977: cli: fix use of IPv6 addresses with RPC client commands r=knz a=knz

Fixes #33008.

(This was actually a regression of my doing, back from #28373. Didn't pick it up back then because we didn't have a test.)

Release note (bug fix): the `cockroach` command line utilities that
internally use a RPC connection (e.g. `cockroach quit`, `cockroach
init`, etc) again properly support passing an IPv6 address literal via
the `--host` argument.

Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@craig

This comment has been minimized.

Copy link

commented Jun 3, 2019

Build succeeded

@craig craig bot merged commit eba3b37 into cockroachdb:master Jun 3, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.