-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Changes from all commits
a25c1c5
f904f08
a3fa7ab
7fb0f26
dfe8e6e
3c92aed
767bcd5
1ca5a3d
96c3ed3
210e641
08cabf1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
/* | ||
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 ( | ||
"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')") | ||
mcmp.Exec(`SELECT name from t1 union select name from t2`) | ||
} |
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) | ||
} |
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; |
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" | ||
} | ||
] | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2398,7 +2398,7 @@ type ( | |
FuncExpr struct { | ||
Qualifier IdentifierCS | ||
Name IdentifierCI | ||
Exprs SelectExprs | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Back in the days, we used Using pure expressions simplifies a lot of code and makes it possible to know that when we are looking at an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Love this change |
||
Exprs Exprs | ||
} | ||
|
||
// ValuesFuncExpr represents a function call. | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 subqueryThere was a problem hiding this comment.
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.There was a problem hiding this comment.
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