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: Add information_schema.statistics #10220

Merged
merged 1 commit into from
Oct 31, 2016

Conversation

yznming
Copy link
Contributor

@yznming yznming commented Oct 26, 2016

#8675 add information_schema.statistics table.


This change is Reviewable

@nvanbenschoten nvanbenschoten self-assigned this Oct 26, 2016
@nvanbenschoten
Copy link
Member

Thanks for putting this together! I have a few comments, but the general approach looks good.


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


pkg/sql/information_schema.go, line 410 at r1 (raw file):

}

var informationSchemaStatisticsTable = virtualSchemaTable{

Lets move this up to be in lexicographical order.


pkg/sql/information_schema.go, line 421 at r1 (raw file):

  SEQ_IN_INDEX INT NOT NULL DEFAULT 0,
  COLUMN_NAME STRING NOT NULL DEFAULT '',
  "COLLATION" STRING NOT NULL DEFAULT '',

Why the quotes?


pkg/sql/information_schema.go, line 430 at r1 (raw file):

          func(db *sqlbase.DatabaseDescriptor, table *sqlbase.TableDescriptor) error {
              appendRow := func(index sqlbase.IndexDescriptor, colName string, sequence int,
                  direction string, isStored bool) error {

direction has a limited set of values (ASC, DESC, N/A). It's probably worth making constants for these values (see tableTypeSystemView)


pkg/sql/information_schema.go, line 432 at r1 (raw file):

                  direction string, isStored bool) error {
                  return addRow(
                      parser.NewDString("def"),                     //table_catalog

Use defString here


pkg/sql/information_schema.go, line 432 at r1 (raw file):

                  direction string, isStored bool) error {
                  return addRow(
                      parser.NewDString("def"),                     //table_catalog

nit: The comments has traditionally had a space after //


pkg/sql/information_schema.go, line 447 at r1 (raw file):

              }

              for _, index := range append([]sqlbase.IndexDescriptor{table.PrimaryIndex}, table.Indexes...) {

We have a line length limit of 100 characters.


pkg/sql/information_schema.go, line 447 at r1 (raw file):

              }

              for _, index := range append([]sqlbase.IndexDescriptor{table.PrimaryIndex}, table.Indexes...) {

See forEachIndexInTable


pkg/sql/information_schema.go, line 449 at r1 (raw file):

              for _, index := range append([]sqlbase.IndexDescriptor{table.PrimaryIndex}, table.Indexes...) {
                  sequence := 1
                  for i, col := range index.ColumnNames {

This deserves a quick comment. Something like "We add a row for each..."


pkg/sql/testdata/information_schema, line 667 at r1 (raw file):

## information_schema.statistics
statement ok
CREATE TABLE other_db.cde(id INT PRIMARY KEY, c INT, d INT, e STRING, INDEX idx_c(c), UNIQUE INDEX idx_cd(c,d))

cde?


Comments from Reviewable

@yznming
Copy link
Contributor Author

yznming commented Oct 27, 2016

pkg/sql/information_schema.go, line 421 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why the quotes?

`collation` is a key word. Create table would execute failed without the quote.
F161027 11:16:38.445281 1 sql/executor.go:289  [startup] syntax error at or near "COLLATION"                                                                       

CREATE TABLE information_schema.statistics (
TABLE_CATALOG STRING NOT NULL DEFAULT '',
TABLE_SCHEMA STRING NOT NULL DEFAULT '',
TABLE_NAME STRING NOT NULL DEFAULT '',
NON_UNIQUE BOOL NOT NULL DEFAULT FALSE,
INDEX_SCHEMA STRING NOT NULL DEFAULT '',
INDEX_NAME STRING NOT NULL DEFAULT '',
SEQ_IN_INDEX INT NOT NULL DEFAULT 0,
COLUMN_NAME STRING NOT NULL DEFAULT '',
COLLATION STRING NOT NULL DEFAULT '',
^


---


*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/10220)*
<!-- Sent from Reviewable.io -->

@yznming
Copy link
Contributor Author

yznming commented Oct 27, 2016

pkg/sql/testdata/information_schema, line 667 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

cde?

sorry, it doesn't means 'Color Doppler Energy' here. I would change the table name .

Comments from Reviewable

@yznming yznming force-pushed the information_schema branch 2 times, most recently from ad83e3a to 25627f7 Compare October 27, 2016 06:48
@nvanbenschoten
Copy link
Member

:lgtm: other than the one final comment. Thanks!


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


pkg/sql/information_schema.go, line 321 at r2 (raw file):

                  }
                  for _, col := range index.StoreColumnNames {
                      // We add a row for each column of index

"for each stored column"

also, punctuation is needed here and above.


Comments from Reviewable

@nvanbenschoten nvanbenschoten merged commit f4a8abc into cockroachdb:master Oct 31, 2016
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.

2 participants