Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions go/vt/topo/topoproto/shard.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,6 @@ func ParseKeyspaceShard(param string) (string, string, error) {
return keySpaceShard[0], keySpaceShard[1], nil
}

// ParseKeyspaceOptionalShard parses a "keyspace/shard" string
// and extracts the parts. If a shard is not specified, it's
// returned as empty string.
func ParseKeyspaceOptionalShard(keyspaceShard string) (string, string) {
last := strings.LastIndex(keyspaceShard, "/")
if last == -1 {
return keyspaceShard, ""
}
return keyspaceShard[:last], keyspaceShard[last+1:]
}

// SourceShardString returns a printable view of a SourceShard.
func SourceShardString(source *topodatapb.Shard_SourceShard) string {
return fmt.Sprintf("SourceShard(%v,%v/%v)", source.Uid, source.Keyspace, source.Shard)
Expand Down
30 changes: 0 additions & 30 deletions go/vt/topo/topoproto/shard_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,36 +31,6 @@ func TestParseKeyspaceShard(t *testing.T) {
}
}

func TestParseKeyspaceOptionalShard(t *testing.T) {
testcases := []struct {
keyspaceShard string
keyspace string
shard string
}{{
keyspaceShard: "ks",
keyspace: "ks",
shard: "",
}, {
keyspaceShard: "/-80",
keyspace: "",
shard: "-80",
}, {
keyspaceShard: "ks/-80",
keyspace: "ks",
shard: "-80",
}, {
keyspaceShard: "ks/",
keyspace: "ks",
shard: "",
}}

for _, tcase := range testcases {
if keyspace, shard := ParseKeyspaceOptionalShard(tcase.keyspaceShard); keyspace != tcase.keyspace || shard != tcase.shard {
t.Errorf("parseKeyspaceShard(%s): %s:%s, want %s:%s", tcase.keyspaceShard, keyspace, shard, tcase.keyspace, tcase.shard)
}
}
}

func TestSourceShardAsHTML(t *testing.T) {
s := &topodatapb.Shard_SourceShard{
Uid: 123,
Expand Down
18 changes: 16 additions & 2 deletions go/vt/vtgate/vtgate.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import (
"github.com/youtube/vitess/go/vt/vtgate/gateway"
"github.com/youtube/vitess/go/vt/vtgate/vtgateservice"

"strings"

querypb "github.com/youtube/vitess/go/vt/proto/query"
topodatapb "github.com/youtube/vitess/go/vt/proto/topodata"
vtgatepb "github.com/youtube/vitess/go/vt/proto/vtgate"
Expand Down Expand Up @@ -248,7 +250,7 @@ func (vtg *VTGate) Execute(ctx context.Context, sql string, bindVariables map[st
vtg.localBegin(session)
}

keyspace, shard := topoproto.ParseKeyspaceOptionalShard(keyspaceShard)
keyspace, shard := parseKeyspaceOptionalShard(keyspaceShard)
if shard != "" {
sql = sqlannotation.AnnotateIfDML(sql, nil)
f := func(keyspace string) (string, []string, error) {
Expand Down Expand Up @@ -578,7 +580,7 @@ func (vtg *VTGate) StreamExecute(ctx context.Context, sql string, bindVariables
statsKey := []string{"StreamExecute", "Any", ltt}
defer vtg.timings.Record(statsKey, startTime)

keyspace, shard := topoproto.ParseKeyspaceOptionalShard(keyspaceShard)
keyspace, shard := parseKeyspaceOptionalShard(keyspaceShard)
var err error
if shard != "" {
err = vtg.resolver.streamExecute(
Expand Down Expand Up @@ -1043,3 +1045,15 @@ func annotateBoundShardQueriesAsUnfriendly(queries []*vtgatepb.BoundShardQuery)
queries[i].Query.Sql = sqlannotation.AnnotateIfDML(q.Query.Sql, nil)
}
}

// parseKeyspaceOptionalShard parses a "keyspace/shard" or "keyspace:shard" string
// and extracts the parts. If a shard is not specified, it's
// returned as empty string. We need to support : and / in vtgate because some clients
// can't support our default of slash. Everywhere else we only support /

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Missing period in the last sentence after /.

func parseKeyspaceOptionalShard(keyspaceShard string) (string, string) {
last := strings.LastIndexAny(keyspaceShard, "/:")
if last == -1 {
return keyspaceShard, ""
}
return keyspaceShard[:last], keyspaceShard[last+1:]
}
42 changes: 42 additions & 0 deletions go/vt/vtgate/vtgate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2683,3 +2683,45 @@ func TestVTGateShowMetadataTwoShards(t *testing.T) {
t.Errorf("shard %s not found in Values \n%+v", shard1, qr.Rows)
}
}

func TestParseKeyspaceOptionalShard(t *testing.T) {
testcases := []struct {
keyspaceShard string
keyspace string
shard string
}{{
keyspaceShard: "ks",
keyspace: "ks",
shard: "",
}, {
keyspaceShard: "/-80",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest that we remove the test cases where one of the parts is empty.

IMO this is outside of the "specification" and therefore the behaviour for such an input should not be defined. But by having it in the test, we're implicitly defining it and making it part of our "specification". For example, we may consider to error on this input in the future.

Therefore, I prefer if you delete this case and the cases ks/, ks: and :-80.

keyspace: "",
shard: "-80",
}, {
keyspaceShard: "ks/-80",
keyspace: "ks",
shard: "-80",
}, {
keyspaceShard: "ks/",
keyspace: "ks",
shard: "",
}, {
keyspaceShard: "ks:",
keyspace: "ks",
shard: "",
}, {
keyspaceShard: "ks:-80",
keyspace: "ks",
shard: "-80",
}, {
keyspaceShard: ":-80",
keyspace: "",
shard: "-80",
}}

for _, tcase := range testcases {
if keyspace, shard := parseKeyspaceOptionalShard(tcase.keyspaceShard); keyspace != tcase.keyspace || shard != tcase.shard {
t.Errorf("parseKeyspaceShard(%s): %s:%s, want %s:%s", tcase.keyspaceShard, keyspace, shard, tcase.keyspace, tcase.shard)
}
}
}