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.

Sign up for GitHub

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

Copy expression types to avoid weight_strings and derived tables #15069

Merged
merged 3 commits into from
Feb 2, 2024
Merged

Copy expression types to avoid weight_strings and derived tables #15069

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b1827e5
copying the types, some Union operators do not need to be transformed…
wangweicugw Jan 29, 2024
f35b917
fix the types of columns on both sides of the union are inconsistent
wangweicugw Jan 31, 2024
f1b01ea
remove else
wangweicugw Jan 31, 2024
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
7 changes: 7 additions & 0 deletions go/test/endtoend/vtgate/queries/union/union_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,4 +130,11 @@ func TestUnion(t *testing.T) {
if utils.BinaryIsAtLeastAtVersion(19, "vtgate") {
mcmp.AssertMatches(`(SELECT id2,'a' from t1 where id1 = 1) union (SELECT 'a',id2 from t1 where id1 = 2)`, `[[VARCHAR("1") VARCHAR("a")] [VARCHAR("a") VARCHAR("2")]]`)
}
mcmp.AssertMatches(`SELECT 1 from t1 UNION SELECT 2 from t1`, `[[INT64(1)] [INT64(2)]]`)
mcmp.AssertMatches(`SELECT 4 from t1 UNION SELECT 3 from t1`, `[[INT64(4)] [INT64(3)]]`)
mcmp.AssertMatches(`SELECT id1 from t1 UNION SELECT id2 from t1`, `[[INT64(1)] [INT64(2)]]`)
mcmp.AssertMatches(`SELECT 1 from t1 UNION SELECT id2 from t1`, `[[INT64(1)] [INT64(2)]]`)
mcmp.AssertMatches(`SELECT 3 from t1 UNION SELECT id2 from t1`, `[[INT64(3)] [INT64(1)] [INT64(2)]]`)
mcmp.AssertMatches(`SELECT id1 from t1 UNION SELECT 2 from t1`, `[[INT64(1)] [INT64(2)]]`)
mcmp.AssertMatches(`SELECT id1 from t1 UNION SELECT 3 from t1`, `[[INT64(1)] [INT64(2)] [INT64(3)]]`)
}
12 changes: 7 additions & 5 deletions go/vt/vtgate/planbuilder/operators/union_merging.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,25 +180,27 @@
cols := make(sqlparser.SelectExprs, len(lhsExprs))
noDeps := len(lhsExprs) != len(rhsExprs)
for idx, col := range lhsExprs {
ae, ok := col.(*sqlparser.AliasedExpr)
lae, ok := col.(*sqlparser.AliasedExpr)

Check warning on line 183 in go/vt/vtgate/planbuilder/operators/union_merging.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/planbuilder/operators/union_merging.go#L183 

Added line #L183 was not covered by tests
if !ok {
cols[idx] = col
noDeps = true
continue
}
col := sqlparser.NewColName(ae.ColumnName())
col := sqlparser.NewColName(lae.ColumnName())

Check warning on line 189 in go/vt/vtgate/planbuilder/operators/union_merging.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/planbuilder/operators/union_merging.go#L189 

Added line #L189 was not covered by tests
cols[idx] = aeWrap(col)
if noDeps {
continue
}

deps := ctx.SemTable.RecursiveDeps(ae.Expr)
ae, ok = rhsExprs[idx].(*sqlparser.AliasedExpr)
deps := ctx.SemTable.RecursiveDeps(lae.Expr)
rae, ok := rhsExprs[idx].(*sqlparser.AliasedExpr)

Check warning on line 196 in go/vt/vtgate/planbuilder/operators/union_merging.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/planbuilder/operators/union_merging.go#L195-L196 

Added lines #L195 - L196 were not covered by tests
if !ok {
noDeps = true
continue
}
deps = deps.Merge(ctx.SemTable.RecursiveDeps(ae.Expr))
deps = deps.Merge(ctx.SemTable.RecursiveDeps(rae.Expr))
ctx.SemTable.CopySemanticInfo(rae.Expr, col)
ctx.SemTable.CopySemanticInfo(lae.Expr, col)

Check warning on line 203 in go/vt/vtgate/planbuilder/operators/union_merging.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/planbuilder/operators/union_merging.go#L201-L203 

Added lines #L201 - L203 were not covered by tests
Copy link
Collaborator

@systay systay Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this is correct. This works when both the left and right columns have the same type, but if they have different types, we should coerce the resulting type and not just copy it.

Here is an example query:

mysql> select 1 union select now();
Field   1:  `1`
Catalog:    `def`
Database:   ``
Table:      ``
Org_table:  ``
Type:       VAR_STRING
Collation:  utf8mb4_0900_ai_ci (255)
Length:     76
Max_length: 19
Decimals:   0
Flags:      NOT_NULL 


+---------------------+
| 1                   |
+---------------------+
| 1                   |
| 2024-01-30 07:54:24 |
+---------------------+
2 rows in set (0.00 sec)

As you can see when you run the mysql client with the option --column-type-info, we have two columns with different types - int and datetime, and they are coerced into varchar.

Fortunately, we already have this functionality today. Here is my suggestion:

Suggested change
ctx.SemTable.CopySemanticInfo(rae.Expr, col)
ctx.SemTable.CopySemanticInfo(lae.Expr, col)
rt, foundR := ctx.SemTable.TypeForExpr(rae.Expr)
lt, foundL := ctx.SemTable.TypeForExpr(lae.Expr)
if foundR && foundL {
types := []sqltypes.Type{rt.Type(), lt.Type()}
t := evalengine.AggregateTypes(types)
ctx.SemTable.ExprTypes[col] = evalengine.NewType(t, collations.Unknown)
}

Copy link
Contributor Author

@wangweicugw wangweicugw Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I overlooked the case where the left and right columns have different types. However, in some cases, you may not be able to retrieve the evalengine.Type through ctx.SemTable.TypeForExpr() method, especially when dealing with syntax tree nodes like now() that cannot provide the evalengine.Type after processing it with ctx.SemTable.TypeForExpr() .
So, I added some additional logic with an "else" condition to handle cases where the evalengine.Type cannot be obtained.

ctx.SemTable.Recursive[col] = deps
}

Expand Down
113 changes: 100 additions & 13 deletions go/vt/vtgate/planbuilder/testdata/union_cases.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,9 +373,8 @@
"Instructions": {
"OperatorType": "Distinct",
"Collations": [
"(0:1)"
"0"
],
"ResultColumns": 1,
"Inputs": [
{
"OperatorType": "Route",
Expand All @@ -384,8 +383,8 @@
"Name": "user",
"Sharded": true
},
"FieldQuery": "select id, weight_string(id) from (select id from `user` where 1 != 1 union select id from music where 1 != 1 union select 1 from dual where 1 != 1) as dt where 1 != 1",
"Query": "select id, weight_string(id) from (select id from `user` union select id from music union select 1 from dual) as dt",
"FieldQuery": "select id from `user` where 1 != 1 union select id from music where 1 != 1 union select 1 from dual where 1 != 1",
"Query": "select id from `user` union select id from music union select 1 from dual",
"Table": "`user`, dual, music"
}
]
Expand Down Expand Up @@ -523,9 +522,8 @@
"Instructions": {
"OperatorType": "Distinct",
"Collations": [
"(0:1)"
"0"
],
"ResultColumns": 1,
"Inputs": [
{
"OperatorType": "Route",
Expand All @@ -534,8 +532,8 @@
"Name": "user",
"Sharded": true
},
"FieldQuery": "select `1`, weight_string(`1`) from (select 1 from dual where 1 != 1 union select null from dual where 1 != 1 union select 1.0 from dual where 1 != 1 union select '1' from dual where 1 != 1 union select 2 from dual where 1 != 1 union select 2.0 from `user` where 1 != 1) as dt where 1 != 1",
"Query": "select `1`, weight_string(`1`) from (select 1 from dual union select null from dual union select 1.0 from dual union select '1' from dual union select 2 from dual union select 2.0 from `user`) as dt",
"FieldQuery": "select 1 from dual where 1 != 1 union select null from dual where 1 != 1 union select 1.0 from dual where 1 != 1 union select '1' from dual where 1 != 1 union select 2 from dual where 1 != 1 union select 2.0 from `user` where 1 != 1",
"Query": "select 1 from dual union select null from dual union select 1.0 from dual union select '1' from dual union select 2 from dual union select 2.0 from `user`",
"Table": "`user`, dual"
}
]
Expand Down Expand Up @@ -836,9 +834,8 @@
{
"OperatorType": "Distinct",
"Collations": [
"(0:1)"
"0"
],
"ResultColumns": 1,
"Inputs": [
{
"OperatorType": "Route",
Expand All @@ -847,8 +844,8 @@
"Name": "user",
"Sharded": true
},
"FieldQuery": "select id, weight_string(id) from (select id from `user` where 1 != 1 union select 3 from dual where 1 != 1) as dt where 1 != 1",
"Query": "select id, weight_string(id) from (select id from `user` union select 3 from dual limit :__upper_limit) as dt",
"FieldQuery": "select id from `user` where 1 != 1 union select 3 from dual where 1 != 1",
"Query": "select id from `user` union select 3 from dual limit :__upper_limit",
"Table": "`user`, dual"
}
]
Expand Down Expand Up @@ -1045,7 +1042,7 @@
{
"OperatorType": "Distinct",
"Collations": [
"(0:1)",
"0",
"1"
],
"Inputs": [
Expand Down Expand Up @@ -1535,5 +1532,95 @@
"user.user"
]
}
},
{
"comment": "Select literals from table union Select literals from table",
"query": "SELECT 1 from user UNION SELECT 2 from user",
"plan": {
"QueryType": "SELECT",
"Original": "SELECT 1 from user UNION SELECT 2 from user",
"Instructions": {
"OperatorType": "Distinct",
"Collations": [
"0"
],
"Inputs": [
{
"OperatorType": "Route",
"Variant": "Scatter",
"Keyspace": {
"Name": "user",
"Sharded": true
},
"FieldQuery": "select 1 from `user` where 1 != 1 union select 2 from `user` where 1 != 1",
"Query": "select 1 from `user` union select 2 from `user`",
"Table": "`user`"
}
]
},
"TablesUsed": [
"user.user"
]
}
},
{
"comment": "Select column from table union Select literals from table",
"query": "select col1 from user union select 3 from user",
"plan": {
"QueryType": "SELECT",
"Original": "select col1 from user union select 3 from user",
"Instructions": {
"OperatorType": "Distinct",
"Collations": [
"0"
],
"Inputs": [
{
"OperatorType": "Route",
"Variant": "Scatter",
"Keyspace": {
"Name": "user",
"Sharded": true
},
"FieldQuery": "select col1 from `user` where 1 != 1 union select 3 from `user` where 1 != 1",
"Query": "select col1 from `user` union select 3 from `user`",
"Table": "`user`"
}
]
},
"TablesUsed": [
"user.user"
]
}
},
{
"comment": "Select literals from table union Select column from table",
"query": "select 3 from user union select col1 from user",
"plan": {
"QueryType": "SELECT",
"Original": "select 3 from user union select col1 from user",
"Instructions": {
"OperatorType": "Distinct",
"Collations": [
"0"
],
"Inputs": [
{
"OperatorType": "Route",
"Variant": "Scatter",
"Keyspace": {
"Name": "user",
"Sharded": true
},
"FieldQuery": "select 3 from `user` where 1 != 1 union select col1 from `user` where 1 != 1",
"Query": "select 3 from `user` union select col1 from `user`",
"Table": "`user`"
}
]
},
"TablesUsed": [
"user.user"
]
}
}
]
Loading