From 4ef70c19da33b06c03e72aadaf9786e4b0a49277 Mon Sep 17 00:00:00 2001 From: pokeeffe-molecula <85502298+pokeeffe-molecula@users.noreply.github.com> Date: Wed, 14 Dec 2022 16:27:55 -0600 Subject: [PATCH] SHOW CREATE TABLE issues (fb-1810) (#2365) * fixed ddl issues with cache type/size; removed shardwidth option; improved error message (cherry picked from commit c88d60c9ab13f5987014fe2acfbc97cd75c8c28e) --- sql3/errors.go | 16 ++++----- sql3/parser/ast.go | 15 -------- sql3/parser/parser.go | 25 +++----------- sql3/parser/token.go | 2 -- sql3/planner/compilecreatetable.go | 22 +++--------- sql3/planner/opcreatetable.go | 2 +- sql3/planner/opsystemtable.go | 24 ++++++++----- sql3/sql_complex_test.go | 53 ++++++++++++++++++++++++----- sql3/test/defs/defs_create_table.go | 27 --------------- 9 files changed, 77 insertions(+), 109 deletions(-) diff --git a/sql3/errors.go b/sql3/errors.go index 694c33252..f8142351f 100644 --- a/sql3/errors.go +++ b/sql3/errors.go @@ -75,10 +75,10 @@ const ( ErrTableIDColumnConstraints errors.Code = "ErrTableIDColumnConstraints" ErrTableIDColumnAlter errors.Code = "ErrTableIDColumnAlter" ErrTableNotFound errors.Code = "ErrTableNotFound" + ErrTableExists errors.Code = "ErrTableExists" ErrColumnNotFound errors.Code = "ErrColumnNotFound" ErrTableColumnNotFound errors.Code = "ErrTableColumnNotFound" ErrInvalidKeyPartitionsValue errors.Code = "ErrInvalidKeyPartitionsValue" - ErrInvalidShardWidthValue errors.Code = "ErrInvalidShardWidthValue" ErrBadColumnConstraint errors.Code = "ErrBadColumnConstraint" ErrConflictingColumnConstraint errors.Code = "ErrConflictingColumnConstraint" @@ -504,6 +504,13 @@ func NewErrTableNotFound(line, col int, tableName string) error { ) } +func NewErrTableExists(line, col int, tableName string) error { + return errors.New( + ErrTableExists, + fmt.Sprintf("[%d:%d] table '%s' already exists", line, col, tableName), + ) +} + func NewErrColumnNotFound(line, col int, columnName string) error { return errors.New( ErrColumnNotFound, @@ -525,13 +532,6 @@ func NewErrInvalidKeyPartitionsValue(line, col int, keypartitions int64) error { ) } -func NewErrInvalidShardWidthValue(line, col int, shardwidth int64) error { - return errors.New( - ErrInvalidShardWidthValue, - fmt.Sprintf("[%d:%d] invalid value '%d' for shardwidth (should be a number that is a power of 2 and greater or equal to 2^16)", line, col, shardwidth), - ) -} - func NewErrBadColumnConstraint(line, col int, constraint, columnType string) error { return errors.New( ErrBadColumnConstraint, diff --git a/sql3/parser/ast.go b/sql3/parser/ast.go index 84bc019fa..01e5a5555 100644 --- a/sql3/parser/ast.go +++ b/sql3/parser/ast.go @@ -78,7 +78,6 @@ func (*ResultColumn) node() {} func (*RollbackStatement) node() {} func (*SavepointStatement) node() {} func (*SelectStatement) node() {} -func (*ShardWidthOption) node() {} func (*StringLit) node() {} func (*TableValuedFunction) node() {} func (*TimeUnitConstraint) node() {} @@ -771,7 +770,6 @@ type TableOption interface { } func (*KeyPartitionsOption) option() {} -func (*ShardWidthOption) option() {} func (*CommentOption) option() {} type KeyPartitionsOption struct { @@ -787,19 +785,6 @@ func (o *KeyPartitionsOption) String() string { return buf.String() } -type ShardWidthOption struct { - ShardWidth Pos // position of SHARDWIDTH keyword - Expr Expr // expression -} - -func (o *ShardWidthOption) String() string { - var buf bytes.Buffer - buf.WriteString("SHARDWIDTH (") - buf.WriteString(o.Expr.String()) - buf.WriteString(")") - return buf.String() -} - type CommentOption struct { Comment Pos // position of COMMENT keyword Expr Expr // expression diff --git a/sql3/parser/parser.go b/sql3/parser/parser.go index 3077bb502..150e7eab3 100644 --- a/sql3/parser/parser.go +++ b/sql3/parser/parser.go @@ -441,15 +441,13 @@ func (p *Parser) parseTableOption() (_ TableOption, err error) { var optionPos Pos - // Parse column constraints. + // Parse table options. switch p.peek() { case KEYPARTITIONS: return p.parseKeyPartitionsOption(optionPos) - case COMMENT: - return p.parseCommentOption(optionPos) default: - assert(p.peek() == SHARDWIDTH) - return p.parseShardWidthOption(optionPos) + assert(p.peek() == COMMENT) + return p.parseCommentOption(optionPos) } } @@ -484,21 +482,6 @@ func (p *Parser) parseKeyPartitionsOption(optionPos Pos) (_ *KeyPartitionsOption return &opt, nil } -func (p *Parser) parseShardWidthOption(optionPos Pos) (_ *ShardWidthOption, err error) { - assert(p.peek() == SHARDWIDTH) - - var opt ShardWidthOption - opt.ShardWidth, _, _ = p.scan() - - if isLiteralToken(p.peek()) { - opt.Expr = p.mustParseLiteral() - } else { - return &opt, p.errorExpected(p.pos, p.tok, "literal") - } - - return &opt, nil -} - func (p *Parser) parseColumnDefinitions() (_ []*ColumnDefinition, err error) { var columns []*ColumnDefinition for { @@ -3436,7 +3419,7 @@ func (e Error) Error() string { // isTableOptionStartToken returns true if tok is the initial token of a table option. func isTableOptionStartToken(tok Token) bool { switch tok { - case KEYPARTITIONS, SHARDWIDTH, COMMENT: + case KEYPARTITIONS, COMMENT: return true default: return false diff --git a/sql3/parser/token.go b/sql3/parser/token.go index b79809c4d..97a4a5f5d 100644 --- a/sql3/parser/token.go +++ b/sql3/parser/token.go @@ -214,7 +214,6 @@ const ( SELECT SELECT_COLUMN SET - SHARDWIDTH SIZE SHOW SPAN @@ -438,7 +437,6 @@ var tokens = [...]string{ SELECT_COLUMN: "SELECT_COLUMN", SET: "SET", SIZE: "SIZE", - SHARDWIDTH: "SHARDWIDTH", SHOW: "SHOW", SPAN: "SPAN", TABLE: "TABLE", diff --git a/sql3/planner/compilecreatetable.go b/sql3/planner/compilecreatetable.go index 70d483043..3a5eb5f34 100644 --- a/sql3/planner/compilecreatetable.go +++ b/sql3/planner/compilecreatetable.go @@ -67,7 +67,11 @@ func (p *ExecutionPlanner) compileCreateTableStatement(stmt *parser.CreateTableS columns = append(columns, column) } - return NewPlanOpQuery(p, NewPlanOpCreateTable(p, tableName, failIfExists, isKeyed, keyPartitions, description, columns), p.sql), nil + cop := NewPlanOpCreateTable(p, tableName, failIfExists, isKeyed, keyPartitions, description, columns) + if keyPartitions > 0 { + cop.AddWarning("The value of KEYPARTITIONS is currently ignored") + } + return NewPlanOpQuery(p, cop, p.sql), nil } // compiles a column def @@ -289,22 +293,6 @@ func (p *ExecutionPlanner) analyzeCreateTableStatement(stmt *parser.CreateTableS return sql3.NewErrInvalidKeyPartitionsValue(o.Expr.Pos().Line, o.Expr.Pos().Column, i) } - case *parser.ShardWidthOption: - //check the type of the expression - literal, ok := o.Expr.(*parser.IntegerLit) - if !ok { - return sql3.NewErrIntegerLiteral(o.Expr.Pos().Line, o.Expr.Pos().Column) - } - //shardwidth needs to be a power of 2 and > 2^16 - i, err := strconv.ParseInt(literal.Value, 10, 64) - if err != nil { - return err - } - isPwrOf2 := (i & (i - 1)) == 0 - if (i == 0) || !isPwrOf2 || i < (1<<16) { - return sql3.NewErrInvalidShardWidthValue(o.Expr.Pos().Line, o.Expr.Pos().Column, i) - } - case *parser.CommentOption: _, ok := o.Expr.(*parser.StringLit) diff --git a/sql3/planner/opcreatetable.go b/sql3/planner/opcreatetable.go index e16e7e76b..3414e44e3 100644 --- a/sql3/planner/opcreatetable.go +++ b/sql3/planner/opcreatetable.go @@ -135,7 +135,7 @@ func (i *createTableRowIter) Next(ctx context.Context) (types.Row, error) { if err := i.planner.schemaAPI.CreateTable(ctx, tbl); err != nil { if _, ok := errors.Cause(err).(pilosa.ConflictError); ok { if i.failIfExists { - return nil, err + return nil, sql3.NewErrTableExists(0, 0, i.tableName) } } else { return nil, err diff --git a/sql3/planner/opsystemtable.go b/sql3/planner/opsystemtable.go index 0e41e64b4..f7a983c27 100644 --- a/sql3/planner/opsystemtable.go +++ b/sql3/planner/opsystemtable.go @@ -68,11 +68,6 @@ var systemTables = map[string]*systemTable{ ColumnName: "node_count", Type: parser.NewDataTypeInt(), }, - &types.PlannerColumn{ - RelationName: fbClusterInfo, - ColumnName: "shard_width", - Type: parser.NewDataTypeInt(), - }, &types.PlannerColumn{ RelationName: fbClusterInfo, ColumnName: "replica_count", @@ -312,7 +307,6 @@ func (i *fbClusterInfoRowIter) Next(ctx context.Context) (types.Row, error) { i.planner.systemAPI.Version(), i.planner.systemAPI.ClusterState(), i.planner.systemAPI.ClusterNodeCount(), - i.planner.systemAPI.ShardWidth(), i.planner.systemAPI.ClusterReplicaCount(), } i.rowIndex += 1 @@ -436,7 +430,11 @@ func (i *fbTableDDLRowIter) Next(ctx context.Context) (types.Row, error) { fmt.Fprintf(&buf, " cachetype %s", col.Options.CacheType) } if col.Options.CacheSize != pilosa.DefaultCacheSize && col.Options.CacheSize > 0 { - fmt.Fprintf(&buf, " cachesize %d", col.Options.CacheSize) + // if we still have the default, we need to print that out if we have a non-default size + if col.Options.CacheType == pilosa.DefaultCacheType && len(col.Options.CacheType) > 0 { + fmt.Fprintf(&buf, " cachetype %s", col.Options.CacheType) + } + fmt.Fprintf(&buf, " size %d", col.Options.CacheSize) } case *parser.DataTypeIDSet, *parser.DataTypeStringSet: @@ -444,7 +442,11 @@ func (i *fbTableDDLRowIter) Next(ctx context.Context) (types.Row, error) { fmt.Fprintf(&buf, " cachetype %s", col.Options.CacheType) } if col.Options.CacheSize != pilosa.DefaultCacheSize && col.Options.CacheSize > 0 { - fmt.Fprintf(&buf, " cachesize %d", col.Options.CacheSize) + // if we still have the default, we need to print that out if we have a non-default size + if col.Options.CacheType == pilosa.DefaultCacheType && len(col.Options.CacheType) > 0 { + fmt.Fprintf(&buf, " cachetype %s", col.Options.CacheType) + } + fmt.Fprintf(&buf, " size %d", col.Options.CacheSize) } case *parser.DataTypeIDSetQuantum, *parser.DataTypeStringSetQuantum: @@ -452,7 +454,11 @@ func (i *fbTableDDLRowIter) Next(ctx context.Context) (types.Row, error) { fmt.Fprintf(&buf, " cachetype %s", col.Options.CacheType) } if col.Options.CacheSize != pilosa.DefaultCacheSize && col.Options.CacheSize > 0 { - fmt.Fprintf(&buf, " cachesize %d", col.Options.CacheSize) + // if we still have the default, we need to print that out if we have a non-default size + if col.Options.CacheType == pilosa.DefaultCacheType && len(col.Options.CacheType) > 0 { + fmt.Fprintf(&buf, " cachetype %s", col.Options.CacheType) + } + fmt.Fprintf(&buf, " size %d", col.Options.CacheSize) } if !col.Options.TimeQuantum.IsEmpty() { fmt.Fprintf(&buf, " timequantum '%s'", col.Options.TimeQuantum) diff --git a/sql3/sql_complex_test.go b/sql3/sql_complex_test.go index 3bbca6d1d..7d7056ace 100644 --- a/sql3/sql_complex_test.go +++ b/sql3/sql_complex_test.go @@ -68,7 +68,7 @@ func TestPlanner_Show(t *testing.T) { } t.Run("SystemTablesInfo", func(t *testing.T) { - results, columns, err := sql_test.MustQueryRows(t, c.GetNode(0).Server, `select name, platform, platform_version, db_version, state, node_count, shard_width, replica_count from fb_cluster_info`) + results, columns, err := sql_test.MustQueryRows(t, c.GetNode(0).Server, `select name, platform, platform_version, db_version, state, node_count, replica_count from fb_cluster_info`) if err != nil { t.Fatal(err) } @@ -83,7 +83,6 @@ func TestPlanner_Show(t *testing.T) { wireQueryFieldString("db_version"), wireQueryFieldString("state"), wireQueryFieldInt("node_count"), - wireQueryFieldInt("shard_width"), wireQueryFieldInt("replica_count"), }, columns); diff != "" { t.Fatal(diff) @@ -204,6 +203,42 @@ func TestPlanner_Show(t *testing.T) { } }) + t.Run("ShowCreateTableCacheTypes", func(t *testing.T) { + _, _, err := sql_test.MustQueryRows(t, c.GetNode(0).Server, `create table iris1 ( + _id id, + speciesid id cachetype ranked size 1000 + species string cachetype ranked size 1000 + speciesids idset cachetype ranked size 1000 + speciess stringset cachetype ranked size 1000 + speciesidsq idset timequantum 'YMD' + speciessq stringset timequantum 'YMD' + ) keypartitions 12 + `) + if err != nil { + t.Fatal(err) + } + + results, columns, err := sql_test.MustQueryRows(t, c.GetNode(0).Server, `SHOW CREATE TABLE iris1`) + if err != nil { + t.Fatal(err) + } + if len(results) != 1 { + t.Fatal(fmt.Errorf("unexpected result set length: %d", len(results))) + } + + if diff := cmp.Diff([][]interface{}{ + {string("create table iris1 (_id id, speciesid id cachetype ranked size 1000, species string cachetype ranked size 1000, speciesids idset cachetype ranked size 1000, speciess stringset cachetype ranked size 1000, speciesidsq idset timequantum 'YMD', speciessq stringset timequantum 'YMD');")}, + }, results); diff != "" { + t.Fatal(diff) + } + + if diff := cmp.Diff([]*pilosa.WireQueryField{ + wireQueryFieldString("ddl"), + }, columns); diff != "" { + t.Fatal(diff) + } + }) + t.Run("ShowColumns", func(t *testing.T) { results, columns, err := sql_test.MustQueryRows(t, c.GetNode(0).Server, fmt.Sprintf(`SHOW COLUMNS FROM %i`, c)) if err != nil { @@ -313,7 +348,7 @@ func TestPlanner_CoverCreateTable(t *testing.T) { // Build the create table statement based on the fields slice above. sql := "create table " + tableName + "_" + fld.name + " (_id id, " sql += fld.name + " " + fld.typ + " " + fld.constraints - sql += `) keypartitions 12 shardwidth 1024` + sql += `) keypartitions 12` // Run the create table statement. _, _, err := sql_test.MustQueryRows(t, server, sql) @@ -478,7 +513,7 @@ func TestPlanner_CoverCreateTable(t *testing.T) { } } sql += strings.Join(fieldDefs, ", ") - sql += `) keypartitions 12 shardwidth 65536` + sql += `) keypartitions 12` // Run the create table statement. results, columns, err := sql_test.MustQueryRows(t, server, sql) @@ -553,7 +588,7 @@ func TestPlanner_CreateTable(t *testing.T) { stringcol string, stringsetcol stringset, idcol id, - idsetcol idset) keypartitions 12 shardwidth 65536`) + idsetcol idset) keypartitions 12`) if err != nil { t.Fatal(err) } @@ -576,11 +611,11 @@ func TestPlanner_CreateTable(t *testing.T) { stringcol string, stringsetcol stringset, idcol id, - idsetcol idset) keypartitions 12 shardwidth 65536`) + idsetcol idset) keypartitions 12`) if err == nil { t.Fatal("expected error") } else { - if err.Error() != "creating index: index already exists" { + if err.Error() != "[0:0] table 'allcoltypes' already exists" { t.Fatal(err) } } @@ -606,7 +641,7 @@ func TestPlanner_CreateTable(t *testing.T) { idcol id cachetype ranked size 1000, idsetcol idset cachetype lru, idsetcolsz idset cachetype lru size 1000, - idsetcolq idset timequantum 'YMD' ttl '24h') keypartitions 12 shardwidth 65536`) + idsetcolq idset timequantum 'YMD' ttl '24h') keypartitions 12`) if err != nil { t.Fatal(err) } @@ -1740,7 +1775,7 @@ func TestPlanner_BulkInsert(t *testing.T) { petallength decimal(2), petalwidth decimal(2), species string cachetype ranked size 1000 - ) keypartitions 12 shardwidth 65536;`) + ) keypartitions 12;`) if err != nil { t.Fatal(err) } diff --git a/sql3/test/defs/defs_create_table.go b/sql3/test/defs/defs_create_table.go index 7d9b8f22f..724dd03f2 100644 --- a/sql3/test/defs/defs_create_table.go +++ b/sql3/test/defs/defs_create_table.go @@ -17,33 +17,6 @@ var createTable = TableTest{ ), ExpErr: "invalid value '10001' for key partitions (should be a number between 1-10000)", }, - { - name: "shardWidthSetTo0", - SQLs: sqls( - "create table foo (_id id, i1 int) shardwidth 0", - ), - ExpErr: "invalid value '0' for shardwidth (should be a number that is a power of 2 and greater or equal to 2^16)", - }, - { - name: "shardWidthSetTo11", - SQLs: sqls( - "create table foo (_id id, i1 int) shardwidth 11", - ), - ExpErr: "invalid value '11' for shardwidth (should be a number that is a power of 2 and greater or equal to 2^16)", - }, - { - name: "shardWidthSetTo11", - SQLs: sqls( - "create table foo (_id id, i1 int) shardwidth 32", - ), - ExpErr: "invalid value '32' for shardwidth (should be a number that is a power of 2 and greater or equal to 2^16)", - }, - { - name: "shardWidthSetTo131072", - SQLs: sqls( - "create table foo (_id id, i1 int) shardwidth 131072", - ), - }, { name: "commentInt", SQLs: sqls(