From 7f75193cf296464361ee66e387b031f13c9b5449 Mon Sep 17 00:00:00 2001 From: Pat Okeeffe <85502298+paddyjok@users.noreply.github.com> Date: Fri, 7 Apr 2023 12:58:09 -0500 Subject: [PATCH] tidy up show tables behavior (#2374) * tidy up show tables behavior * made cli integration test whole again * Update fbsql \d meta-command to show system tables (#2376) --------- Co-authored-by: Travis Turner --- cli/meta.go | 15 ++- cli/testdata/table | 40 ++++--- sql3/errors.go | 12 ++ sql3/parser/ast.go | 10 +- sql3/parser/parser.go | 8 +- sql3/parser/parser_test.go | 10 ++ sql3/planner/compileshow.go | 16 ++- sql3/planner/executionplannersystemtables.go | 6 + sql3/planner/opfeaturebasetables.go | 112 +++++++++++-------- sql3/sql_complex_test.go | 36 ++++++ 10 files changed, 199 insertions(+), 66 deletions(-) diff --git a/cli/meta.go b/cli/meta.go index 74d1816c7..dc22b3724 100644 --- a/cli/meta.go +++ b/cli/meta.go @@ -364,7 +364,7 @@ Input/Output \warn [-n] [STRING] write string to standard error (-n for no newline) Informational - \d list tables + \d list tables, including system tables \d NAME describe table \dt list tables \dv list views @@ -504,8 +504,17 @@ func (m *metaDescribe) execute(cmd *Command) (responseAction, error) { switch len(m.args) { case 0: // Describe with no args should list all relations (tables, views, - // etc.). For now, we're just going to list the tables. - return newMetaListTables().execute(cmd) + // etc.). For now, we're just going to list the tables, including system + // tables. + qry := []queryPart{ + newPartRaw("SHOW TABLES WITH SYSTEM"), + } + + if err := cmd.executeAndWriteQuery(qry); err != nil { + return actionNone, errors.Wrap(err, "executing query") + } + + return actionReset, nil case 1: // Describe with a single arg will assume the arg is a table name, so it diff --git a/cli/testdata/table b/cli/testdata/table index c2f2e2752..713a4aaf8 100644 --- a/cli/testdata/table +++ b/cli/testdata/table @@ -1,5 +1,5 @@ -// Show tables for database using SHOW TABLES. -SEND:SHOW TABLES; +// Show tables for database using SHOW TABLES WITH SYSTEM. +SEND:SHOW TABLES WITH SYSTEM; EXPECT:+-------------------------+-------------------------+-------+------------+----------------------+----------------------+-------+------------+-------------+ EXPECT:| _id | name | owner | updated_by | created_at | updated_at | keys | space_used | description | EXPECT:+-------------------------+-------------------------+-------+------------+----------------------+----------------------+-------+------------+-------------+ @@ -11,8 +11,8 @@ EXPECTCOMP:WithFormat:| fb_____________________ | fb_____________________ | EXPECT:+-------------------------+-------------------------+-------+------------+----------------------+----------------------+-------+------------+-------------+ EXPECT: -// Show tables for database using \dt. -SEND:\dt +// Show tables for database using \d. +SEND:\d EXPECT:+-------------------------+-------------------------+-------+------------+----------------------+----------------------+-------+------------+-------------+ EXPECT:| _id | name | owner | updated_by | created_at | updated_at | keys | space_used | description | EXPECT:+-------------------------+-------------------------+-------+------------+----------------------+----------------------+-------+------------+-------------+ @@ -24,6 +24,23 @@ EXPECTCOMP:WithFormat:| fb_____________________ | fb_____________________ | EXPECT:+-------------------------+-------------------------+-------+------------+----------------------+----------------------+-------+------------+-------------+ EXPECT: +// Show tables for database using SHOW TABLES. +SEND:SHOW TABLES; +EXPECT:+-----+------+-------+------------+------------+------------+------+------------+-------------+ +EXPECT:| _id | name | owner | updated_by | created_at | updated_at | keys | space_used | description | +EXPECT:+-----+------+-------+------------+------------+------------+------+------------+-------------+ +EXPECT:+-----+------+-------+------------+------------+------------+------+------------+-------------+ +EXPECT: + + +// Show tables for database using \dt. +SEND:\dt +EXPECT:+-----+------+-------+------------+------------+------------+------+------------+-------------+ +EXPECT:| _id | name | owner | updated_by | created_at | updated_at | keys | space_used | description | +EXPECT:+-----+------+-------+------------+------------+------------+------+------------+-------------+ +EXPECT:+-----+------+-------+------------+------------+------------+------+------------+-------------+ +EXPECT: + // Create a table. That can be used for general testing. SEND:CREATE TABLE users (_id id, name string, age int); EXPECT: @@ -33,16 +50,11 @@ EXPECT: // Show tables for database to get the newly created table. SEND:\dt -EXPECT:+-------------------------+-------------------------+-------+------------+----------------------+----------------------+-------+------------+-------------+ -EXPECT:| _id | name | owner | updated_by | created_at | updated_at | keys | space_used | description | -EXPECT:+-------------------------+-------------------------+-------+------------+----------------------+----------------------+-------+------------+-------------+ -EXPECTCOMP:WithFormat:| users | users | | | {timestamp} | {timestamp} | false | 0 | | -EXPECTCOMP:WithFormat:| fb_____________________ | fb_____________________ | | | {timestamp} | {timestamp} | false | 0 | | -EXPECTCOMP:WithFormat:| fb_____________________ | fb_____________________ | | | {timestamp} | {timestamp} | false | 0 | | -EXPECTCOMP:WithFormat:| fb_____________________ | fb_____________________ | | | {timestamp} | {timestamp} | false | 0 | | -EXPECTCOMP:WithFormat:| fb_____________________ | fb_____________________ | | | {timestamp} | {timestamp} | false | 0 | | -EXPECTCOMP:WithFormat:| fb_____________________ | fb_____________________ | | | {timestamp} | {timestamp} | false | 0 | | -EXPECT:+-------------------------+-------------------------+-------+------------+----------------------+----------------------+-------+------------+-------------+ +EXPECT:+-------+-------+-------+------------+----------------------+----------------------+-------+------------+-------------+ +EXPECT:| _id | name | owner | updated_by | created_at | updated_at | keys | space_used | description | +EXPECT:+-------+-------+-------+------------+----------------------+----------------------+-------+------------+-------------+ +EXPECTCOMP:WithFormat:| users | users | | | {timestamp} | {timestamp} | false | 0 | | +EXPECT:+-------+-------+-------+------------+----------------------+----------------------+-------+------------+-------------+ EXPECT: // We don't select from users until AFTER we check SHOW TABLES above because diff --git a/sql3/errors.go b/sql3/errors.go index 4ea363954..676534fcb 100644 --- a/sql3/errors.go +++ b/sql3/errors.go @@ -152,6 +152,9 @@ const ( // query hints ErrUnknownQueryHint errors.Code = "ErrInvalidQueryHint" ErrInvalidQueryHintParameterCount errors.Code = "ErrInvalidQueryHintParameterCount" + + // show options + ErrUnknownShowOption errors.Code = "ErrUnknownShowOption" ) func NewErrDuplicateColumn(line int, col int, column string) error { @@ -931,3 +934,12 @@ func NewErrInvalidQueryHintParameterCount(line, col int, hintName string, desire fmt.Sprintf("[%d:%d] query hint '%s' expected %d parameter(s) (%s), got %d parameters", line, col, hintName, desiredCount, desiredList, actualCount), ) } + +// show options + +func NewErrUnknownShowOption(line, col int, optionName string) error { + return errors.New( + ErrUnknownShowOption, + fmt.Sprintf("[%d:%d] unknown show option '%s'", line, col, optionName), + ) +} diff --git a/sql3/parser/ast.go b/sql3/parser/ast.go index aa63e6e4c..17f8738b2 100644 --- a/sql3/parser/ast.go +++ b/sql3/parser/ast.go @@ -540,11 +540,19 @@ func (s *ShowDatabasesStatement) Clone() *ShowDatabasesStatement { type ShowTablesStatement struct { Show Pos // position of SHOW Tables Pos // position of TABLES + With Pos + System *Ident } // String returns the string representation of the statement. func (s *ShowTablesStatement) String() string { - return "SHOW TABLES" + var buf bytes.Buffer + buf.WriteString("SHOW TABLES") + if s.With.IsValid() { + buf.WriteString(" WITH") + fmt.Fprintf(&buf, " %s", s.System.String()) + } + return buf.String() } func (s *ShowTablesStatement) Clone() *ShowTablesStatement { diff --git a/sql3/parser/parser.go b/sql3/parser/parser.go index 6649adee2..50533472f 100644 --- a/sql3/parser/parser.go +++ b/sql3/parser/parser.go @@ -176,12 +176,18 @@ func (p *Parser) parseShowDatabasesStatement(showPos Pos) (*ShowDatabasesStateme } } -func (p *Parser) parseShowTablesStatement(showPos Pos) (*ShowTablesStatement, error) { +func (p *Parser) parseShowTablesStatement(showPos Pos) (_ *ShowTablesStatement, err error) { switch p.peek() { case TABLES: var stmt ShowTablesStatement stmt.Show = showPos stmt.Tables, _, _ = p.scan() + if p.peek() == WITH { + stmt.With, _, _ = p.scan() + if stmt.System, err = p.parseIdent("show tables option"); err != nil { + return &stmt, err + } + } return &stmt, nil default: return nil, p.errorExpected(p.pos, p.tok, "TABLES") diff --git a/sql3/parser/parser_test.go b/sql3/parser/parser_test.go index 5d6651e0a..b818a1956 100644 --- a/sql3/parser/parser_test.go +++ b/sql3/parser/parser_test.go @@ -658,8 +658,18 @@ func TestParser_ParseStatement(t *testing.T) { Show: pos(0), Tables: pos(5), }) + AssertParseStatement(t, `SHOW TABLES WITH SYSTEM`, &parser.ShowTablesStatement{ + Show: pos(0), + Tables: pos(5), + With: pos(12), + System: &parser.Ident{ + Name: "SYSTEM", + NamePos: pos(17), + }, + }) AssertParseStatementError(t, `SHOW`, `1:4: expected DATABASES, TABLES, COLUMNS or CREATE, found 'EOF'`) AssertParseStatementError(t, `SHOW BLAH`, `1:6: expected DATABASES, TABLES, COLUMNS or CREATE, found BLAH`) + AssertParseStatementError(t, `SHOW TABLES WITH`, `1:16: expected show tables option, found 'EOF'`) }) t.Run("ShowColumns", func(t *testing.T) { diff --git a/sql3/planner/compileshow.go b/sql3/planner/compileshow.go index b70aa6129..fb3748727 100644 --- a/sql3/planner/compileshow.go +++ b/sql3/planner/compileshow.go @@ -73,7 +73,19 @@ func (p *ExecutionPlanner) compileShowDatabasesStatement(ctx context.Context, st return NewPlanOpQuery(p, NewPlanOpProjection(columns, NewPlanOpFeatureBaseDatabases(p, dbs)), p.sql), nil } -func (p *ExecutionPlanner) compileShowTablesStatement(ctx context.Context, stmt parser.Statement) (types.PlanOperator, error) { +func (p *ExecutionPlanner) compileShowTablesStatement(ctx context.Context, stmt *parser.ShowTablesStatement) (types.PlanOperator, error) { + + showSystem := false + if stmt.With.IsValid() { + opt := parser.IdentName(stmt.System) + + if !strings.EqualFold("system", opt) { + return nil, sql3.NewErrUnknownShowOption(stmt.System.NamePos.Line, stmt.System.NamePos.Column, opt) + } + + showSystem = true + } + tbls, err := p.schemaAPI.Tables(ctx) if err != nil { return nil, errors.Wrap(err, "getting tables") @@ -135,7 +147,7 @@ func (p *ExecutionPlanner) compileShowTablesStatement(ctx context.Context, stmt dataType: parser.NewDataTypeString(), }} - return NewPlanOpQuery(p, NewPlanOpProjection(columns, NewPlanOpFeatureBaseTables(p, pilosa.TablesToIndexInfos(tbls))), p.sql), nil + return NewPlanOpQuery(p, NewPlanOpProjection(columns, NewPlanOpFeatureBaseTables(p, pilosa.TablesToIndexInfos(tbls), showSystem)), p.sql), nil } func (p *ExecutionPlanner) compileShowColumnsStatement(ctx context.Context, stmt *parser.ShowColumnsStatement) (_ types.PlanOperator, err error) { diff --git a/sql3/planner/executionplannersystemtables.go b/sql3/planner/executionplannersystemtables.go index da8338047..70bfd889b 100644 --- a/sql3/planner/executionplannersystemtables.go +++ b/sql3/planner/executionplannersystemtables.go @@ -4,6 +4,7 @@ package planner import ( "context" + "sort" "time" pilosa "github.com/featurebasedb/featurebase/v3" @@ -81,6 +82,11 @@ func (s *systemTableDefinitionsWrapper) Tables(ctx context.Context) ([]*dax.Tabl tbls = append(tbls, pilosa.IndexInfoToTable(ii)) } + // order the result - by ID asc right now + sort.Slice(tbls, func(i, j int) bool { + return tbls[i].ID < tbls[j].ID + }) + return tbls, nil } diff --git a/sql3/planner/opfeaturebasetables.go b/sql3/planner/opfeaturebasetables.go index 00ea42b98..99d50c973 100644 --- a/sql3/planner/opfeaturebasetables.go +++ b/sql3/planner/opfeaturebasetables.go @@ -17,16 +17,18 @@ import ( // PlanOpFeatureBaseTables wraps a []*IndexInfo that is returned from // schemaAPI.Schema(). type PlanOpFeatureBaseTables struct { - planner *ExecutionPlanner - indexInfo []*pilosa.IndexInfo - warnings []string + planner *ExecutionPlanner + indexInfo []*pilosa.IndexInfo + withSystem bool + warnings []string } -func NewPlanOpFeatureBaseTables(planner *ExecutionPlanner, indexInfo []*pilosa.IndexInfo) *PlanOpFeatureBaseTables { +func NewPlanOpFeatureBaseTables(planner *ExecutionPlanner, indexInfo []*pilosa.IndexInfo, withSystem bool) *PlanOpFeatureBaseTables { return &PlanOpFeatureBaseTables{ - planner: planner, - indexInfo: indexInfo, - warnings: make([]string, 0), + planner: planner, + indexInfo: indexInfo, + withSystem: withSystem, + warnings: make([]string, 0), } } @@ -105,65 +107,85 @@ func (p *PlanOpFeatureBaseTables) Children() []types.PlanOperator { func (p *PlanOpFeatureBaseTables) Iterator(ctx context.Context, row types.Row) (types.RowIterator, error) { return &showTablesRowIter{ - planner: p.planner, - indexInfo: p.indexInfo, + planner: p.planner, + indexInfo: p.indexInfo, + withSystem: p.withSystem, }, nil } func (p *PlanOpFeatureBaseTables) WithChildren(children ...types.PlanOperator) (types.PlanOperator, error) { - return NewPlanOpFeatureBaseTables(p.planner, p.indexInfo), nil + return NewPlanOpFeatureBaseTables(p.planner, p.indexInfo, p.withSystem), nil } type showTablesRowIter struct { - planner *ExecutionPlanner - indexInfo []*pilosa.IndexInfo - rowIndex int + planner *ExecutionPlanner + indexInfo []*pilosa.IndexInfo + withSystem bool + + result types.Rows } var _ types.RowIterator = (*showTablesRowIter)(nil) func (i *showTablesRowIter) Next(ctx context.Context) (types.Row, error) { - if i.rowIndex < len(i.indexInfo) { + if i.result == nil { + i.result = make(types.Rows, 0) - indexName := i.indexInfo[i.rowIndex].Name + for _, idx := range i.indexInfo { - var err error - var spaceUsed pilosa.DiskUsage - switch strings.ToLower(indexName) { - case fbDatabaseInfo, fbDatabaseNodes, fbPerformanceCounters, fbExecRequests, fbTableDDL: - spaceUsed = pilosa.DiskUsage{ - Usage: 0, + indexName := idx.Name + + // if we don't want system tables filter them out (currently by name prefix) + // TODO(pok) - we need an is_system attribute so we can filter on that instead + if !i.withSystem && strings.HasPrefix(indexName, "fb_") { + continue } - default: - u := i.planner.systemAPI.DataDir() - // TODO(tlt): GetDiskUsage needs to be behind an interface because - // this doesn't work in serverless. For now I'm just going to skip - // it based on the emtpy DataDir, but let's do this the right way. - if u != "" { - u = fmt.Sprintf("%s/indexes/%s", u, indexName) + var err error + var spaceUsed pilosa.DiskUsage + switch strings.ToLower(indexName) { + case fbDatabaseInfo, fbDatabaseNodes, fbPerformanceCounters, fbExecRequests, fbTableDDL: + spaceUsed = pilosa.DiskUsage{ + Usage: 0, + } + default: + u := i.planner.systemAPI.DataDir() - spaceUsed, err = pilosa.GetDiskUsage(u) - if err != nil { - return nil, err + // TODO(tlt): GetDiskUsage needs to be behind an interface because + // this doesn't work in serverless. For now I'm just going to skip + // it based on the emtpy DataDir, but let's do this the right way. + if u != "" { + u = fmt.Sprintf("%s/indexes/%s", u, indexName) + + spaceUsed, err = pilosa.GetDiskUsage(u) + if err != nil { + return nil, err + } } } - } - createdAt := time.Unix(0, i.indexInfo[i.rowIndex].CreatedAt) - updatedAt := time.Unix(0, i.indexInfo[i.rowIndex].UpdatedAt) - row := []interface{}{ - indexName, - indexName, - i.indexInfo[i.rowIndex].Owner, - i.indexInfo[i.rowIndex].LastUpdateUser, - createdAt.Format(time.RFC3339), - updatedAt.Format(time.RFC3339), - i.indexInfo[i.rowIndex].Options.Keys, - spaceUsed.Usage, - i.indexInfo[i.rowIndex].Options.Description, + createdAt := time.Unix(0, idx.CreatedAt) + updatedAt := time.Unix(0, idx.UpdatedAt) + row := []interface{}{ + indexName, + indexName, + idx.Owner, + idx.LastUpdateUser, + createdAt.Format(time.RFC3339), + updatedAt.Format(time.RFC3339), + idx.Options.Keys, + spaceUsed.Usage, + idx.Options.Description, + } + i.result = append(i.result, row) } - i.rowIndex += 1 + } + + if len(i.result) > 0 { + row := i.result[0] + + // Move to next result element. + i.result = i.result[1:] return row, nil } return nil, types.ErrNoMoreRows diff --git a/sql3/sql_complex_test.go b/sql3/sql_complex_test.go index 05b55a65e..5629a8199 100644 --- a/sql3/sql_complex_test.go +++ b/sql3/sql_complex_test.go @@ -266,6 +266,42 @@ func TestPlanner_Show(t *testing.T) { } }) + t.Run("ShowTablesWithBadOption", func(t *testing.T) { + _, _, _, err := sql_test.MustQueryRows(t, nil, c.GetNode(0).Server, `SHOW TABLES WITH SCOTCH`) + if err != nil { + if err.Error() != "[1:18] unknown show option 'SCOTCH'" { + t.Fatal(err) + } + } + }) + + t.Run("ShowTablesWithSystem", func(t *testing.T) { + results, columns, _, err := sql_test.MustQueryRows(t, nil, c.GetNode(0).Server, `SHOW TABLES WITH SYSTEM`) + if err != nil { + t.Fatal(err) + } + + // we keep adding system tables on the fly so as + // long as we get more than 0 tables, we're good + if len(results) == 0 { + t.Fatal(fmt.Errorf("unexpected result set length")) + } + + if diff := cmp.Diff([]*pilosa.WireQueryField{ + wireQueryFieldString("_id"), + wireQueryFieldString("name"), + wireQueryFieldString("owner"), + wireQueryFieldString("updated_by"), + wireQueryFieldTimestamp("created_at"), + wireQueryFieldTimestamp("updated_at"), + wireQueryFieldBool("keys"), + wireQueryFieldInt("space_used"), + wireQueryFieldString("description"), + }, columns); diff != "" { + t.Fatal(diff) + } + }) + t.Run("ShowCreateTable", func(t *testing.T) { results, columns, _, err := sql_test.MustQueryRows(t, nil, c.GetNode(0).Server, fmt.Sprintf(`SHOW CREATE TABLE %i`, c)) if err != nil {