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 <travis@molecula.com>

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

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

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),
+	)
+}

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 {

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")

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) {

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) {

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
 }
 

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)
+
+		for _, idx := range i.indexInfo {
 
-		indexName := i.indexInfo[i.rowIndex].Name
+			indexName := idx.Name
 
-		var err error
-		var spaceUsed pilosa.DiskUsage
-		switch strings.ToLower(indexName) {
-		case fbDatabaseInfo, fbDatabaseNodes, fbPerformanceCounters, fbExecRequests, fbTableDDL:
-			spaceUsed = pilosa.DiskUsage{
-				Usage: 0,
+			// 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)
-
-				spaceUsed, err = pilosa.GetDiskUsage(u)
-				if err != nil {
-					return nil, err
+
+			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()
+
+				// 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