Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: Normalizing literals in SELECT is broken #15043

Closed
wants to merge 11 commits into from
66 changes: 66 additions & 0 deletions go/test/endtoend/vtgate/queries/collations/collations_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
Copyright 2024 The Vitess Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package collations

import (
"fmt"
"testing"

"github.com/stretchr/testify/require"

"vitess.io/vitess/go/test/endtoend/cluster"
"vitess.io/vitess/go/test/endtoend/utils"
)

func start(t *testing.T) (utils.MySQLCompare, func()) {
// ensure that the vschema and the tables have been created before running any tests
_ = utils.WaitForAuthoritative(t, keyspaceName, "t1", clusterInstance.VtgateProcess.ReadVSchema)
_ = utils.WaitForAuthoritative(t, keyspaceName, "t2", clusterInstance.VtgateProcess.ReadVSchema)

mcmp, err := utils.NewMySQLCompare(t, vtParams, mysqlParams)
require.NoError(t, err)

deleteAll := func() {
_, _ = utils.ExecAllowError(t, mcmp.VtConn, "set workload = oltp")

tables := []string{"t1", "t2"}
for _, table := range tables {
_, _ = mcmp.ExecAndIgnore("delete from " + table)
}
}

deleteAll()

return mcmp, func() {
deleteAll()
mcmp.Close()
cluster.PanicHandler(t)
}
}

func TestCollationTyping(t *testing.T) {
utils.SkipIfBinaryIsBelowVersion(t, 19, "vtgate")

mcmp, closer := start(t)
defer closer()
mcmp.Exec("insert into t1(id, name) values(1,'ß'), (2,'s'), (3,'ss')")
mcmp.Exec("insert into t2(id, name) values(1,'ß'), (2,'s'), (3,'ss')")

res := utils.Exec(t, mcmp.VtConn, `vexplain plan SELECT name from t1 union select name from t2`)
fmt.Printf("%v", res.Rows)
systay marked this conversation as resolved.
Show resolved Hide resolved
mcmp.Exec(`SELECT name from t1 union select name from t2`)
}
94 changes: 94 additions & 0 deletions go/test/endtoend/vtgate/queries/collations/main_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/*
Copyright 2024 The Vitess Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package collations

import (
_ "embed"
"flag"
"fmt"
"os"
"testing"

"vitess.io/vitess/go/test/endtoend/utils"

"vitess.io/vitess/go/mysql"
"vitess.io/vitess/go/test/endtoend/cluster"
)

var (
clusterInstance *cluster.LocalProcessCluster
vtParams mysql.ConnParams
mysqlParams mysql.ConnParams
keyspaceName = "ks_aggr"
cell = "test_aggr"

//go:embed schema.sql
schemaSQL string

//go:embed vschema.json
vschema string
)

func TestMain(m *testing.M) {
defer cluster.PanicHandler(nil)
flag.Parse()

exitCode := func() int {
clusterInstance = cluster.NewCluster(cell, "localhost")
defer clusterInstance.Teardown()

// Start topo server
err := clusterInstance.StartTopo()
if err != nil {
return 1
}

// Start keyspace
keyspace := &cluster.Keyspace{
Name: keyspaceName,
SchemaSQL: schemaSQL,
VSchema: vschema,
}
clusterInstance.VtGateExtraArgs = []string{"--schema_change_signal"}
clusterInstance.VtTabletExtraArgs = []string{"--queryserver-config-schema-change-signal"}
err = clusterInstance.StartKeyspace(*keyspace, []string{"-80", "80-"}, 0, false)
if err != nil {
return 1
}

clusterInstance.VtGateExtraArgs = append(clusterInstance.VtGateExtraArgs, "--enable_system_settings=true")
// Start vtgate
err = clusterInstance.StartVtgate()
if err != nil {
return 1
}

vtParams = clusterInstance.GetVTParams(keyspaceName)

// create mysql instance and connection parameters
conn, closer, err := utils.NewMySQL(clusterInstance, keyspaceName, schemaSQL)
if err != nil {
fmt.Println(err)
return 1
}
defer closer()
mysqlParams = conn

return m.Run()
}()
os.Exit(exitCode)
}
13 changes: 13 additions & 0 deletions go/test/endtoend/vtgate/queries/collations/schema.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
create table t1
(
id bigint,
name varchar(255) collate utf8_general_mysql500_ci,
primary key (id)
) Engine = InnoDB;

create table t2
(
id bigint,
name varchar(255) collate utf8_general_mysql500_ci,
primary key (id)
) ENGINE = InnoDB;
29 changes: 29 additions & 0 deletions go/test/endtoend/vtgate/queries/collations/vschema.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
{
"sharded": true,
"vindexes": {
"hash": {
"type": "hash"
},
"unicode_loose_xxhash": {
"type": "unicode_loose_xxhash"
}
},
"tables": {
"t1": {
"column_vindexes": [
{
"column": "id",
"name": "hash"
}
]
},
"t2": {
"column_vindexes": [
{
"column": "id",
"name": "hash"
}
]
}
}
}
5 changes: 4 additions & 1 deletion go/test/endtoend/vtgate/queries/subquery/subquery_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,10 @@ func TestSubqueryInAggregation(t *testing.T) {
mcmp.Exec("insert into t1(id1, id2) values(0,0),(1,1)")
mcmp.Exec("insert into t2(id3, id4) values(1,2),(5,7)")
mcmp.Exec(`SELECT max((select min(id2) from t1)) FROM t2`)
mcmp.Exec(`SELECT max((select group_concat(id1, id2) from t1 where id1 = 1)) FROM t1 where id1 = 1`)

// we need an alias for the subquery so that the id1 = 1 on the inside and the outside look the same and can be merged
mcmp.Exec(`SELECT max((select group_concat(id1, id2) from t1 where id1 = 1)) as x FROM t1 where id1 = 1`)
Copy link
Member

Choose a reason for hiding this comment

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

why an alias is needed here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without the alias we will not normalize the subquery, since it is in a SELECT expression. We are however normalizing the WHERE clause, so the planner doesn't know that the outer query and the inner query are going to the same shard. That turns this into a correlated query that we don't support. By adding the alias, we are free to normalize both and the planner knows it safe to merge the two sides of the subquery

Copy link
Member

Choose a reason for hiding this comment

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

can you add a failing query to the unsupported list if it is not already there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not very easy to do. this is unsupported only after the normalizer has had a go at it. it's unsupported because it's a correlated subquery when we can't merge the two sides, and we already have other examples of correlated subqueries not being supported at the moment


mcmp.Exec(`SELECT max((select min(id2) from t1 where id2 = 1)) FROM dual`)
mcmp.Exec(`SELECT max((select min(id2) from t1)) FROM t2 where id4 = 7`)

Expand Down
2 changes: 1 addition & 1 deletion go/vt/sqlparser/ast.go
Original file line number Diff line number Diff line change
Expand Up @@ -2398,7 +2398,7 @@ type (
FuncExpr struct {
Qualifier IdentifierCS
Name IdentifierCI
Exprs SelectExprs
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Back in the days, we used FuncExpr for all expressions, including count(*). Since then, we have replaced most functions with custom AST types (such as CountStar), and this means that we no longer need to parse * as an argument to a function call.

Using pure expressions simplifies a lot of code and makes it possible to know that when we are looking at an *sqlparser.AliasedExpr, we know we are in the SELECT clause and nowhere else.

Copy link
Member

Choose a reason for hiding this comment

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

Love this change

Exprs Exprs
}

// ValuesFuncExpr represents a function call.
Expand Down
2 changes: 1 addition & 1 deletion go/vt/sqlparser/ast_clone.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions go/vt/sqlparser/ast_copy_on_rewrite.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion go/vt/sqlparser/ast_equals.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions go/vt/sqlparser/ast_rewrite.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion go/vt/sqlparser/ast_visit.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion go/vt/sqlparser/cached_size.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions go/vt/sqlparser/normalizer.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,9 @@ import (

"vitess.io/vitess/go/mysql/datetime"
"vitess.io/vitess/go/sqltypes"
querypb "vitess.io/vitess/go/vt/proto/query"
vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
"vitess.io/vitess/go/vt/vterrors"

querypb "vitess.io/vitess/go/vt/proto/query"
)

// BindVars is a set of reserved bind variables from a SQL statement
Expand Down Expand Up @@ -133,6 +132,9 @@ func (nz *normalizer) walkDownSelect(node, parent SQLNode) bool {
case *ConvertType:
// we should not rewrite the type description
return false
case *AliasedExpr:
// we only want to rewrite literals in select expressions if the column is aliased
return node.As.NotEmpty()
}
return nz.err == nil // only continue if we haven't found any errors
}
Expand Down
Loading
Loading