Skip to content

Commit

Permalink
chore(spanner/spansql): use ID type for identifiers throughout (#2889)
Browse files Browse the repository at this point in the history 
This was a long-planned cleanup, which adds some rigour to the type
definitions, and avoids a bunch of unnecessary casting in various
places.
  • Loading branch information
@dsymonds
dsymonds committed Sep 18, 2020
1 parent b4d938e commit 80f55c7
Show file tree
Hide file tree
Showing 10 changed files with 123 additions and 116 deletions.
42 changes: 21 additions & 21 deletions spanner/spannertest/db.go
View file
Expand Up @@ -43,8 +43,8 @@ import (
type database struct {
mu sync.Mutex
lastTS time.Time // last commit timestamp
tables map[string]*table
indexes map[string]struct{} // only record their existence
tables map[spansql.ID]*table
indexes map[spansql.ID]struct{} // only record their existence

rwMu sync.Mutex // held by read-write transactions
}
Expand All @@ -55,17 +55,17 @@ type table struct {
// Information about the table columns.
// They are reordered on table creation so the primary key columns come first.
cols []colInfo
colIndex map[string]int // col name to index
pkCols int // number of primary key columns (may be 0)
pkDesc []bool // whether each primary key column is in descending order
colIndex map[spansql.ID]int // col name to index
pkCols int // number of primary key columns (may be 0)
pkDesc []bool // whether each primary key column is in descending order

// Rows are stored in primary key order.
rows []row
}

// colInfo represents information about a column in a table or result set.
type colInfo struct {
Name string
Name spansql.ID
Type spansql.Type
AggIndex int // Index+1 of SELECT list for which this is an aggregate value.
}
Expand Down Expand Up @@ -239,10 +239,10 @@ func (d *database) ApplyDDL(stmt spansql.DDLStmt) *status.Status {

// Lazy init.
if d.tables == nil {
d.tables = make(map[string]*table)
d.tables = make(map[spansql.ID]*table)
}
if d.indexes == nil {
d.indexes = make(map[string]struct{})
d.indexes = make(map[spansql.ID]struct{})
}

switch stmt := stmt.(type) {
Expand All @@ -259,7 +259,7 @@ func (d *database) ApplyDDL(stmt spansql.DDLStmt) *status.Status {
// TODO: check stmt.Interleave details.

// Move primary keys first, preserving their order.
pk := make(map[string]int)
pk := make(map[spansql.ID]int)
var pkDesc []bool
for i, kp := range stmt.PrimaryKey {
pk[kp.Column] = -1000 + i
Expand All @@ -271,7 +271,7 @@ func (d *database) ApplyDDL(stmt spansql.DDLStmt) *status.Status {
})

t := &table{
colIndex: make(map[string]int),
colIndex: make(map[spansql.ID]int),
pkCols: len(pk),
pkDesc: pkDesc,
}
Expand Down Expand Up @@ -334,7 +334,7 @@ func (d *database) ApplyDDL(stmt spansql.DDLStmt) *status.Status {

}

func (d *database) table(tbl string) (*table, error) {
func (d *database) table(tbl spansql.ID) (*table, error) {
d.mu.Lock()
defer d.mu.Unlock()

Expand All @@ -346,7 +346,7 @@ func (d *database) table(tbl string) (*table, error) {
}

// writeValues executes a write option (Insert, Update, etc.).
func (d *database) writeValues(tx *transaction, tbl string, cols []string, values []*structpb.ListValue, f func(t *table, colIndexes []int, r row) error) error {
func (d *database) writeValues(tx *transaction, tbl spansql.ID, cols []spansql.ID, values []*structpb.ListValue, f func(t *table, colIndexes []int, r row) error) error {
if err := tx.checkMutable(); err != nil {
return err
}
Expand Down Expand Up @@ -406,7 +406,7 @@ func (d *database) writeValues(tx *transaction, tbl string, cols []string, value
return nil
}

func (d *database) Insert(tx *transaction, tbl string, cols []string, values []*structpb.ListValue) error {
func (d *database) Insert(tx *transaction, tbl spansql.ID, cols []spansql.ID, values []*structpb.ListValue) error {
return d.writeValues(tx, tbl, cols, values, func(t *table, colIndexes []int, r row) error {
pk := r[:t.pkCols]
rowNum, found := t.rowForPK(pk)
Expand All @@ -418,7 +418,7 @@ func (d *database) Insert(tx *transaction, tbl string, cols []string, values []*
})
}

func (d *database) Update(tx *transaction, tbl string, cols []string, values []*structpb.ListValue) error {
func (d *database) Update(tx *transaction, tbl spansql.ID, cols []spansql.ID, values []*structpb.ListValue) error {
return d.writeValues(tx, tbl, cols, values, func(t *table, colIndexes []int, r row) error {
if t.pkCols == 0 {
return status.Errorf(codes.InvalidArgument, "cannot update table %s with no columns in primary key", tbl)
Expand All @@ -437,7 +437,7 @@ func (d *database) Update(tx *transaction, tbl string, cols []string, values []*
})
}

func (d *database) InsertOrUpdate(tx *transaction, tbl string, cols []string, values []*structpb.ListValue) error {
func (d *database) InsertOrUpdate(tx *transaction, tbl spansql.ID, cols []spansql.ID, values []*structpb.ListValue) error {
return d.writeValues(tx, tbl, cols, values, func(t *table, colIndexes []int, r row) error {
pk := r[:t.pkCols]
rowNum, found := t.rowForPK(pk)
Expand All @@ -456,7 +456,7 @@ func (d *database) InsertOrUpdate(tx *transaction, tbl string, cols []string, va

// TODO: Replace

func (d *database) Delete(tx *transaction, table string, keys []*structpb.ListValue, keyRanges keyRangeList, all bool) error {
func (d *database) Delete(tx *transaction, table spansql.ID, keys []*structpb.ListValue, keyRanges keyRangeList, all bool) error {
if err := tx.checkMutable(); err != nil {
return err
}
Expand Down Expand Up @@ -507,7 +507,7 @@ func (d *database) Delete(tx *transaction, table string, keys []*structpb.ListVa
}

// readTable executes a read option (Read, ReadAll).
func (d *database) readTable(table string, cols []string, f func(*table, *rawIter, []int) error) (*rawIter, error) {
func (d *database) readTable(table spansql.ID, cols []spansql.ID, f func(*table, *rawIter, []int) error) (*rawIter, error) {
t, err := d.table(table)
if err != nil {
return nil, err
Expand All @@ -528,7 +528,7 @@ func (d *database) readTable(table string, cols []string, f func(*table, *rawIte
return ri, f(t, ri, colIndexes)
}

func (d *database) Read(tbl string, cols []string, keys []*structpb.ListValue, keyRanges keyRangeList, limit int64) (rowIter, error) {
func (d *database) Read(tbl spansql.ID, cols []spansql.ID, keys []*structpb.ListValue, keyRanges keyRangeList, limit int64) (rowIter, error) {
// The real Cloud Spanner returns an error if the key set is empty by definition.
// That doesn't seem to be well-defined, but it is a common error to attempt a read with no keys,
// so catch that here and return a representative error.
Expand Down Expand Up @@ -592,7 +592,7 @@ func (d *database) Read(tbl string, cols []string, keys []*structpb.ListValue, k
})
}

func (d *database) ReadAll(tbl string, cols []string, limit int64) (*rawIter, error) {
func (d *database) ReadAll(tbl spansql.ID, cols []spansql.ID, limit int64) (*rawIter, error) {
return d.readTable(tbl, cols, func(t *table, ri *rawIter, colIndexes []int) error {
for _, r := range t.rows {
ri.add(r, colIndexes)
Expand Down Expand Up @@ -631,7 +631,7 @@ func (t *table) addColumn(cd spansql.ColumnDef, newTable bool) *status.Status {
return nil
}

func (t *table) dropColumn(name string) *status.Status {
func (t *table) dropColumn(name spansql.ID) *status.Status {
// Only permit dropping non-key columns that aren't part of a secondary index.
// We don't support indexes, so only check that it isn't part of the primary key.

Expand Down Expand Up @@ -753,7 +753,7 @@ func (t *table) findRange(r *keyRange) (int, int) {
}

// colIndexes returns the indexes for the named columns.
func (t *table) colIndexes(cols []string) ([]int, error) {
func (t *table) colIndexes(cols []spansql.ID) ([]int, error) {
var is []int
for _, col := range cols {
i, ok := t.colIndex[col]
Expand Down
14 changes: 7 additions & 7 deletions spanner/spannertest/db_eval.go
View file
Expand Up @@ -37,7 +37,7 @@ type evalContext struct {
row row

// If there are visible aliases, they are populated here.
aliases map[string]spansql.Expr
aliases map[spansql.ID]spansql.Expr

params queryParams
}
Expand Down Expand Up @@ -451,23 +451,23 @@ func (ec evalContext) evalExpr(e spansql.Expr) (interface{}, error) {

func (ec evalContext) evalID(id spansql.ID) (interface{}, error) {
for i, col := range ec.cols {
if col.Name == string(id) {
if col.Name == id {
return ec.row.copyDataElem(i), nil
}
}
if e, ok := ec.aliases[string(id)]; ok {
if e, ok := ec.aliases[id]; ok {
// Make a copy of the context without this alias
// to prevent an evaluation cycle.
innerEC := ec
innerEC.aliases = make(map[string]spansql.Expr)
innerEC.aliases = make(map[spansql.ID]spansql.Expr)
for alias, e := range ec.aliases {
if alias != string(id) {
if alias != id {
innerEC.aliases[alias] = e
}
}
return innerEC.evalExpr(e)
}
return nil, fmt.Errorf("couldn't resolve identifier %s", string(id))
return nil, fmt.Errorf("couldn't resolve identifier %s", id)
}

func (ec evalContext) coerceComparisonOpArgs(co spansql.ComparisonOp) (spansql.ComparisonOp, error) {
Expand Down Expand Up @@ -684,7 +684,7 @@ func (ec evalContext) colInfo(e spansql.Expr) (colInfo, error) {
case spansql.ID:
// TODO: support more than only naming a table column.
for _, col := range ec.cols {
if col.Name == string(e) {
if col.Name == e {
return col, nil
}
}
Expand Down
6 changes: 3 additions & 3 deletions spanner/spannertest/db_query.go
View file
Expand Up @@ -269,7 +269,7 @@ type queryParam struct {
Type spansql.Type
}

type queryParams map[string]queryParam
type queryParams map[string]queryParam // TODO: change key to spansql.Param?

func (d *database) Query(q spansql.Query, params queryParams) (rowIter, error) {
// If there's an ORDER BY clause, extend the query to include the expressions we need
Expand Down Expand Up @@ -377,7 +377,7 @@ func (d *database) evalSelect(sel spansql.Select, params queryParams) (ri rowIte
var rowGroups [][2]int // Sequence of half-open intervals of row numbers.
if len(sel.GroupBy) > 0 {
// Load aliases visible to this GROUP BY.
ec.aliases = make(map[string]spansql.Expr)
ec.aliases = make(map[spansql.ID]spansql.Expr)
for i, alias := range sel.ListAliases {
ec.aliases[alias] = sel.List[i]
}
Expand Down Expand Up @@ -525,7 +525,7 @@ func (d *database) evalSelect(sel spansql.Select, params queryParams) (ri rowIte
aggType = int64Type
}
rawOut.cols = append(raw.cols, colInfo{
Name: fexpr.SQL(),
Name: spansql.ID(fexpr.SQL()), // TODO: this is a bit hokey, but it is output only
Type: aggType,
AggIndex: aggI + 1,
})
Expand Down
32 changes: 16 additions & 16 deletions spanner/spannertest/db_test.go
View file
Expand Up @@ -63,7 +63,7 @@ func TestTableCreation(t *testing.T) {
{Name: "Cool", Type: spansql.Type{Base: spansql.Bool}},
{Name: "Height", Type: spansql.Type{Base: spansql.Float64}},
},
colIndex: map[string]int{
colIndex: map[spansql.ID]int{
"Tenure": 2, "ID": 1, "Cool": 3, "Name": 0, "Height": 4,
},
pkCols: 2,
Expand All @@ -89,7 +89,7 @@ func TestTableData(t *testing.T) {
// Insert a subset of columns.
tx := db.NewTransaction()
tx.Start()
err := db.Insert(tx, "Staff", []string{"ID", "Name", "Tenure", "Height"}, []*structpb.ListValue{
err := db.Insert(tx, "Staff", []spansql.ID{"ID", "Name", "Tenure", "Height"}, []*structpb.ListValue{
// int64 arrives as a decimal string.
listV(stringV("1"), stringV("Jack"), stringV("10"), floatV(1.85)),
listV(stringV("2"), stringV("Daniel"), stringV("11"), floatV(1.83)),
Expand All @@ -98,7 +98,7 @@ func TestTableData(t *testing.T) {
t.Fatalf("Inserting data: %v", err)
}
// Insert a different set of columns.
err = db.Insert(tx, "Staff", []string{"Name", "ID", "Cool", "Tenure", "Height"}, []*structpb.ListValue{
err = db.Insert(tx, "Staff", []spansql.ID{"Name", "ID", "Cool", "Tenure", "Height"}, []*structpb.ListValue{
listV(stringV("Sam"), stringV("3"), boolV(false), stringV("9"), floatV(1.75)),
listV(stringV("Teal'c"), stringV("4"), boolV(true), stringV("8"), floatV(1.91)),
listV(stringV("George"), stringV("5"), nullV(), stringV("6"), floatV(1.73)),
Expand All @@ -113,7 +113,7 @@ func TestTableData(t *testing.T) {
t.Fatalf("Deleting a row: %v", err)
}
// Turns out this guy isn't cool after all.
err = db.Update(tx, "Staff", []string{"Name", "ID", "Cool"}, []*structpb.ListValue{
err = db.Update(tx, "Staff", []spansql.ID{"Name", "ID", "Cool"}, []*structpb.ListValue{
// Missing columns should be left alone.
listV(stringV("Daniel"), stringV("2"), boolV(false)),
})
Expand All @@ -125,7 +125,7 @@ func TestTableData(t *testing.T) {
}

// Read some specific keys.
ri, err := db.Read("Staff", []string{"Name", "Tenure"}, []*structpb.ListValue{
ri, err := db.Read("Staff", []spansql.ID{"Name", "Tenure"}, []*structpb.ListValue{
listV(stringV("George"), stringV("5")),
listV(stringV("Harry"), stringV("6")), // Missing key should be silently ignored.
listV(stringV("Sam"), stringV("3")),
Expand All @@ -143,7 +143,7 @@ func TestTableData(t *testing.T) {
t.Errorf("Read data by keys wrong.\n got %v\nwant %v", all, wantAll)
}
// Read the same, but by key range.
ri, err = db.Read("Staff", []string{"Name", "Tenure"}, nil, keyRangeList{
ri, err = db.Read("Staff", []spansql.ID{"Name", "Tenure"}, nil, keyRangeList{
{start: listV(stringV("Gabriel")), end: listV(stringV("Harpo"))}, // open/open
{
// closed/open
Expand All @@ -162,7 +162,7 @@ func TestTableData(t *testing.T) {
}

// Read a subset of all rows, with a limit.
ri, err = db.ReadAll("Staff", []string{"Tenure", "Name", "Height"}, 4)
ri, err = db.ReadAll("Staff", []spansql.ID{"Tenure", "Name", "Height"}, 4)
if err != nil {
t.Fatalf("ReadAll: %v", err)
}
Expand Down Expand Up @@ -209,7 +209,7 @@ func TestTableData(t *testing.T) {
}
tx = db.NewTransaction()
tx.Start()
err = db.Update(tx, "Staff", []string{"Name", "ID", "FirstSeen", "To"}, []*structpb.ListValue{
err = db.Update(tx, "Staff", []spansql.ID{"Name", "ID", "FirstSeen", "To"}, []*structpb.ListValue{
listV(stringV("Jack"), stringV("1"), stringV("1994-10-28"), nullV()),
listV(stringV("Daniel"), stringV("2"), stringV("1994-10-28"), nullV()),
listV(stringV("George"), stringV("5"), stringV("1997-07-27"), stringV("2008-07-29T11:22:43Z")),
Expand All @@ -225,7 +225,7 @@ func TestTableData(t *testing.T) {
// The queries below ensure that this was all deleted.
tx = db.NewTransaction()
tx.Start()
err = db.Insert(tx, "Staff", []string{"Name", "ID"}, []*structpb.ListValue{
err = db.Insert(tx, "Staff", []spansql.ID{"Name", "ID"}, []*structpb.ListValue{
listV(stringV("01"), stringV("1")),
listV(stringV("03"), stringV("3")),
listV(stringV("06"), stringV("6")),
Expand All @@ -245,7 +245,7 @@ func TestTableData(t *testing.T) {
t.Fatalf("Committing changes: %v", err)
}
// Re-add the data and delete with DML.
err = db.Insert(tx, "Staff", []string{"Name", "ID"}, []*structpb.ListValue{
err = db.Insert(tx, "Staff", []spansql.ID{"Name", "ID"}, []*structpb.ListValue{
listV(stringV("01"), stringV("1")),
listV(stringV("03"), stringV("3")),
listV(stringV("06"), stringV("6")),
Expand Down Expand Up @@ -292,7 +292,7 @@ func TestTableData(t *testing.T) {
}
tx = db.NewTransaction()
tx.Start()
err = db.Update(tx, "Staff", []string{"Name", "ID", "RawBytes"}, []*structpb.ListValue{
err = db.Update(tx, "Staff", []spansql.ID{"Name", "ID", "RawBytes"}, []*structpb.ListValue{
// bytes {0x01 0x00 0x01} encode as base-64 AQAB.
listV(stringV("Jack"), stringV("1"), stringV("AQAB")),
})
Expand Down Expand Up @@ -324,7 +324,7 @@ func TestTableData(t *testing.T) {
}
tx = db.NewTransaction()
tx.Start()
err = db.Insert(tx, "PlayerStats", []string{"LastName", "OpponentID", "PointsScored"}, []*structpb.ListValue{
err = db.Insert(tx, "PlayerStats", []spansql.ID{"LastName", "OpponentID", "PointsScored"}, []*structpb.ListValue{
listV(stringV("Adams"), stringV("51"), stringV("3")),
listV(stringV("Buchanan"), stringV("77"), stringV("0")),
listV(stringV("Coolidge"), stringV("77"), stringV("1")),
Expand Down Expand Up @@ -607,7 +607,7 @@ func TestTableDescendingKey(t *testing.T) {

tx := db.NewTransaction()
tx.Start()
err := db.Insert(tx, "Timeseries", []string{"Name", "Observed", "Value"}, []*structpb.ListValue{
err := db.Insert(tx, "Timeseries", []spansql.ID{"Name", "Observed", "Value"}, []*structpb.ListValue{
listV(stringV("box"), stringV("1"), floatV(1.1)),
listV(stringV("cupcake"), stringV("1"), floatV(6)),
listV(stringV("box"), stringV("2"), floatV(1.2)),
Expand Down Expand Up @@ -665,7 +665,7 @@ func TestTableSchemaConvertNull(t *testing.T) {
// Populate with data including a NULL for the STRING field.
tx := db.NewTransaction()
tx.Start()
err := db.Insert(tx, "Songwriters", []string{"ID", "Nickname"}, []*structpb.ListValue{
err := db.Insert(tx, "Songwriters", []spansql.ID{"ID", "Nickname"}, []*structpb.ListValue{
listV(stringV("6"), stringV("Tiger")),
listV(stringV("7"), nullV()),
})
Expand Down Expand Up @@ -814,7 +814,7 @@ func TestConcurrentReadInsert(t *testing.T) {
// Insert some initial data.
tx := db.NewTransaction()
tx.Start()
err := db.Insert(tx, "Tablino", []string{"A"}, []*structpb.ListValue{
err := db.Insert(tx, "Tablino", []spansql.ID{"A"}, []*structpb.ListValue{
listV(stringV("1")),
listV(stringV("2")),
listV(stringV("4")),
Expand Down Expand Up @@ -850,7 +850,7 @@ func TestConcurrentReadInsert(t *testing.T) {

tx := db.NewTransaction()
tx.Start()
err := db.Insert(tx, "Tablino", []string{"A"}, []*structpb.ListValue{
err := db.Insert(tx, "Tablino", []spansql.ID{"A"}, []*structpb.ListValue{
listV(stringV("3")),
})
if err != nil {
Expand Down

0 comments on commit 80f55c7

Please sign in to comment.