dolthub/vitess

Vitess is a database clustering system for horizontal scaling of MySQL.

vitess's Issues

Vitess protocol buffer namespace conflict

Overview of the Issue

Pulling in both this Vitess fork and the normal Vitess repo simultaneously will cause protocol buffer namespace conflicts. This results in a runtime panic that looks like

panic: proto: file "vttime.proto" has a name conflict over vttime.Time
See https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict


goroutine 1 [running]:
google.golang.org/protobuf/reflect/protoregistry.glob..func1({0x1792700, 0xc000451580}, {0x1792700, 0xc000451580})
        /Users/ryanpbrewster/go/pkg/mod/google.golang.org/[email protected]/reflect/protoregistry/registry.go:54 +0x1f4
google.golang.org/protobuf/reflect/protoregistry.(*Files).RegisterFile.func1({0x17b5d08, 0xc0003ad860})
        /Users/ryanpbrewster/go/pkg/mod/google.golang.org/[email protected]/reflect/protoregistry/registry.go:152 +0x28f
google.golang.org/protobuf/reflect/protoregistry.rangeTopLevelDescriptors({0x17bb488, 0xc000321180}, 0xc00039d600)
        /Users/ryanpbrewster/go/pkg/mod/google.golang.org/[email protected]/reflect/protoregistry/registry.go:415 +0x154
google.golang.org/protobuf/reflect/protoregistry.(*Files).RegisterFile(0xc0001b2018, {0x17bb488, 0xc000321180})
        /Users/ryanpbrewster/go/pkg/mod/google.golang.org/[email protected]/reflect/protoregistry/registry.go:147 +0x745
google.golang.org/protobuf/internal/filedesc.Builder.Build({{0x15add42, 0x23}, {0x1b99e80, 0xc5, 0xc5}, 0x0, 0x2, 0x0, 0x0, {0x179b010, ...}, ...})
        /Users/ryanpbrewster/go/pkg/mod/google.golang.org/[email protected]/internal/filedesc/build.go:113 +0x1d6
google.golang.org/protobuf/internal/filetype.Builder.Build({{{0x15add42, 0x23}, {0x1b99e80, 0xc5, 0xc5}, 0x0, 0x2, 0x0, 0x0, {0x0, ...}, ...}, ...})
        /Users/ryanpbrewster/go/pkg/mod/google.golang.org/[email protected]/internal/filetype/build.go:139 +0x198
vitess.io/vitess/go/vt/proto/vttime.file_vttime_proto_init()
        /Users/ryanpbrewster/go/pkg/mod/vitess.io/[email protected]/go/vt/proto/vttime/vttime.pb.go:239 +0x198
vitess.io/vitess/go/vt/proto/vttime.init.0()
        /Users/ryanpbrewster/go/pkg/mod/vitess.io/[email protected]/go/vt/proto/vttime/vttime.pb.go:195 +0x17
FAIL    command-line-arguments  0.240s
FAIL

Reproduction Steps

I have a single-source reproduction of this issue:

package my_test

import (
  "fmt"
  "testing"

  "github.com/dolthub/go-mysql-server/memory"
  topodatapb "vitess.io/vitess/go/vt/proto/topodata"
)

func Test_Collision(t *testing.T) {
  fmt.Println(memory.NewDatabase("mydb"))
  fmt.Println(topodatapb.CellInfo{})
}

panics with the error message posted at the top of this issue.

There are two possibilities I can see to fix this:

1. Refactor this repo to depend on the upstream Vitess protos instead of vendoring them. This a bit limiting.
2. "shade" these protos by giving them a different package name. Today these proto files look like

syntax = "proto3";
option go_package = "github.com/dolthub/vitess/go/vt/proto/vttime";

package vttime;

and I think if that were switched to package dolthub.vitess.vttime; instead, it would resolve the conflicts.

Operating system and Environment details

The associated go.mod file looks like

module rpb.dev/vitess-proto-sample

go 1.17

require (
  github.com/cespare/xxhash v1.1.0 // indirect
  github.com/davecgh/go-spew v1.1.1 // indirect
  github.com/dolthub/go-mysql-server v0.11.0 // indirect
  github.com/dolthub/vitess v0.0.0-20211013185428-a8845fb919c1 // indirect
  github.com/go-kit/kit v0.9.0 // indirect
  github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b // indirect
  github.com/golang/protobuf v1.5.2 // indirect
  github.com/google/uuid v1.2.0 // indirect
  github.com/hashicorp/golang-lru v0.5.3 // indirect
  github.com/konsorten/go-windows-terminal-sequences v1.0.2 // indirect
  github.com/lestrrat-go/strftime v1.0.1 // indirect
  github.com/mitchellh/hashstructure v1.0.0 // indirect
  github.com/oliveagle/jsonpath v0.0.0-20180606110733-2e52cf6e6852 // indirect
  github.com/opentracing/opentracing-go v1.1.0 // indirect
  github.com/pkg/errors v0.9.1 // indirect
  github.com/pmezard/go-difflib v1.0.0 // indirect
  github.com/shopspring/decimal v0.0.0-20191130220710-360f2bc03045 // indirect
  github.com/sirupsen/logrus v1.4.2 // indirect
  github.com/src-d/go-oniguruma v1.1.0 // indirect
  github.com/stretchr/testify v1.7.0 // indirect
  golang.org/x/net v0.0.0-20210825183410-e898025ed96a // indirect
  golang.org/x/sync v0.0.0-20210220032951-036812b2e83c // indirect
  golang.org/x/sys v0.0.0-20210823070655-63515b42dcdf // indirect
  google.golang.org/genproto v0.0.0-20210701191553-46259e63a0a9 // indirect
  google.golang.org/grpc v1.39.0 // indirect
  google.golang.org/protobuf v1.27.1 // indirect
  gopkg.in/src-d/go-errors.v1 v1.0.0 // indirect
  gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect
  vitess.io/vitess v0.13.1 // indirect
)

Get rid of StrictDDL / PartialDDL distinction in sqlparser package.

The existing parser in go/vt/sqlparser will parse partial DDL statements in some cases and return the partially parsed tree.

This functionality is not needed by go-mysql-server and it adds complexity in a number of places. We should get rid of it.

`alter table <table> add column` with multiple columns not supported

Example query:

create table t (pk int primary key);
alter table t add column (col1 int, col2 int);
desc t;

MySql output:

Field	Type	Null	Key	Default	Extra
pk	int	NO	PRI	NULL	
col1	int	YES		NULL	
col2	int	YES		NULL	

Our output:

Error parsing SQL
syntax error at position 36 near 'int'
alter table t add column (col1 int, col2 int)
                                    ^

The offending rule in sql.y:

alter_table_statement_part:
  ADD column_opt '(' column_definition ')'

Vitess parses the parentheses but expects only a single element within them.

Remove package-level Vitess flags

Similar to #155, this is a show-stopper issue that crops up anytime you use go-mysql-server alongside with Vitess.

I do not believe the dolthub uses of Vitess require these to be flags at all. I do not believe you set them anywhere,

I am going to take a two-pronged approach here:

1. I'll set up a personal repo to demonstrate the flag namespace conflict
2. I'll open a PR against this vitess fork to replace all of the configurable flags with variables hard-coded to the default value

Please let me know if you think the above approach is unlikely to be accepted into this repo.

Extension mechanism for the parser

First of all thank you for an excellent solution!

Feature Description

Have you thought of an extension mechanism for the parser? As far as I know, yacc does not allow extending the grammar. I have a custom storage engine for go-mysql-server that require some small changes to the tokenizer and parser and it seems my only option is to fork your version of vitess?

Use Case(s)

Custom storage engine that does not follow ANSI or mysql SQL syntax.

CREATE TABLE ... WITH ... SELECT not supported

Example query:

CREATE TABLE test
WITH ab AS (
    SELECT 1 as a, 2 as b
)
SELECT * from ab

Our response:

Error parsing SQL:
syntax error at position 23 near 'WITH'
CREATE TABLE test
WITH ab AS (
     ^

The MySQL documentation says this is explicitly allowed (https://dev.mysql.com/doc/refman/8.0/en/with.html) and it works in MySql.