Copy expression types to avoid weight_strings and derived tables #15069

wangweicugw · 2024-01-29T16:14:29Z

Description

The query

select 1 from user 
union 
select 2 from user

was transformed into

select `1`, weight_string(`1`) from (
  select 1 from `user` 
  union 
  select 2 from `user`) as dt

for execution, but by copying the types, it can be simplified and avoid the generation of a derived table. We can now just use the original query.

Related Issue(s)

#15020

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

… into derived tables. Signed-off-by: wangweicugw <[email protected]>

vitess-bot · 2024-01-29T16:14:32Z

systay · 2024-01-30T06:47:17Z

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

+		ctx.SemTable.CopySemanticInfo(rae.Expr, col)
+		ctx.SemTable.CopySemanticInfo(lae.Expr, col)


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)

}

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.

codecov · 2024-01-30T06:53:19Z

Codecov Report

Attention: 488 lines in your changes are missing coverage. Please review.

Comparison is base (eddb39e) 47.29% compared to head (f1b01ea) 47.71%.
Report is 86 commits behind head on main.

Files	Patch %	Lines
...vt/vtgate/planbuilder/operators/sharded_routing.go	0.00%	42 Missing ⚠️
go/vt/vtgate/planbuilder/operators/delete.go	0.00%	32 Missing ⚠️
go/vt/vtgate/evalengine/cached_size.go	0.00%	31 Missing ⚠️
.../vtgate/planbuilder/operators/delete_with_input.go	0.00%	31 Missing ⚠️
go/vt/vtctl/workflow/traffic_switcher.go	0.00%	25 Missing ⚠️
go/vt/vtgate/engine/delete_with_input.go	53.70%	23 Missing and 2 partials ⚠️
...tgate/planbuilder/operators/aggregation_pushing.go	0.00%	25 Missing ⚠️
...vt/vtgate/planbuilder/operators/queryprojection.go	0.00%	20 Missing ⚠️
.../vt/vtgate/planbuilder/operators/query_planning.go	0.00%	19 Missing ⚠️
go/vt/vtgate/evalengine/expr_tuple_bvar.go	62.50%	14 Missing and 4 partials ⚠️
... and 48 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15069      +/-   ##
==========================================
+ Coverage   47.29%   47.71%   +0.41%     
==========================================
  Files        1137     1155      +18     
  Lines      238684   240236    +1552     
==========================================
+ Hits       112895   114621    +1726     
+ Misses     117168   117009     -159     
+ Partials     8621     8606      -15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

systay · 2024-01-30T07:43:56Z

We run the end-to-end tests on older binaries as well. Since this fix hasn't made it to the older versions, we need to stop the test from running if we are not on main. You need to extract the new test lines into it's own test method, and start this method with:

utils.SkipIfBinaryIsBelowVersion(t, 19, "vtgate")

This will stop the Query Serving (Queries) - Upgrade Downgrade Testing / Run Upgrade Downgrade Test CI step from failing

Signed-off-by: wangweicugw <[email protected]>

wangweicugw · 2024-01-31T06:25:28Z

We run the end-to-end tests on older binaries as well. Since this fix hasn't made it to the older versions, we need to stop the test from running if we are not on main. You need to extract the new test lines into it's own test method, and start this method with:
utils.SkipIfBinaryIsBelowVersion(t, 19, "vtgate")
This will stop the Query Serving (Queries) - Upgrade Downgrade Testing / Run Upgrade Downgrade Test CI step from failing

Ok👌🏻

systay · 2024-01-31T06:59:37Z

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

+			types := []sqltypes.Type{rt.Type(), lt.Type()}
+			t := evalengine.AggregateTypes(types)
+			ctx.SemTable.ExprTypes[col] = evalengine.NewType(t, collations.Unknown)
+		} else {


I don't think we should have the else clause here. It's not correct to use either of the two types as the common type. Like my example showed, you can have a situation where one side is an int, the other side is a datetime, but the common type is varchar.

If we don't have both types available, I think we need to use the weight_string function to be sure we can compare values correctly.

I understand now. There shouldn't be an "else" processing logic.

If it is a syntax tree node for the now() function, we can save its corresponding evalengine.Type in ExprTypes during the analysis phase in the analyzer.

Yeah, we could/should update typer.go and add more expressions there. Or, even better, start using the excellent typing logic we have in the evalengine. One of these days...

Signed-off-by: wangweicugw <[email protected]>

dbussink · 2024-02-02T12:40:50Z

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

+		if foundR && foundL {
+			types := []sqltypes.Type{rt.Type(), lt.Type()}
+			t := evalengine.AggregateTypes(types)
+			ctx.SemTable.ExprTypes[col] = evalengine.NewType(t, collations.Unknown)


@systay Shouldn't this have used AggregateEvalTypes so that the collation is also known and not collations.Unknown?

Ah, #15122 answers my question here.

…oid weight_strings and derived tables (vitessio#15069) Signed-off-by: wangweicugw <[email protected]>

…oid weight_strings and derived tables (#15069) (#15129) Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

copying the types, some Union operators do not need to be transformed…

b1827e5

… into derived tables. Signed-off-by: wangweicugw <[email protected]>

wangweicugw requested review from harshit-gangal, systay, frouioui, GuptaManan100 and arthurschreiber as code owners January 29, 2024 16:14

github-actions bot added this to the v19.0.0 milestone Jan 29, 2024

This comment was marked as resolved.

Sign in to view

systay reviewed Jan 30, 2024

View reviewed changes

systay changed the title ~~copying the types, some Union operators do not need to be transformed into derived tables.~~ Copy expression types to avoid weight_strings and derived tables Jan 30, 2024

vmg mentioned this pull request Jan 30, 2024

evalengine: implement AggregateEvalTypes #15085

Merged

5 tasks

fix the types of columns on both sides of the union are inconsistent

f35b917

Signed-off-by: wangweicugw <[email protected]>

systay reviewed Jan 31, 2024

View reviewed changes

remove else

f1b01ea

Signed-off-by: wangweicugw <[email protected]>

harshit-gangal approved these changes Feb 1, 2024

View reviewed changes

systay approved these changes Feb 2, 2024

View reviewed changes

systay merged commit ea8a90d into vitessio:main Feb 2, 2024
102 checks passed

vitess-bot bot mentioned this pull request Feb 2, 2024

[release-18.0] Copy expression types to avoid weight_strings and derived tables (#15069) #15121

Closed

systay mentioned this pull request Feb 2, 2024

feat: use collation aware typing for UNION #15122

Merged

5 tasks

dbussink reviewed Feb 2, 2024

View reviewed changes

wangweicugw deleted the fix_union branch February 5, 2024 00:50

wangweicugw added a commit to wangweicugw/vitess that referenced this pull request Feb 5, 2024

[release-18.0] In the same sqltypes.Type, Copy expression types to av…

6d9400f

…oid weight_strings and derived tables (vitessio#15069) Signed-off-by: wangweicugw <[email protected]>

wangweicugw mentioned this pull request Feb 5, 2024

[release-18.0] In the same sqltypes.Type, Copy expression types to avoid weight_strings and derived tables (#15069) #15129

Merged

5 tasks

systay pushed a commit that referenced this pull request Feb 5, 2024

[release-18.0] In the same sqltypes.Type, Copy expression types to av…

5ac69ec

…oid weight_strings and derived tables (#15069) (#15129) Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy expression types to avoid weight_strings and derived tables #15069

Copy expression types to avoid weight_strings and derived tables #15069

wangweicugw commented Jan 29, 2024 •

edited by systay

Loading

vitess-bot bot commented Jan 29, 2024

This comment was marked as resolved.

This comment was marked as resolved.

systay Jan 30, 2024 •

edited

Loading

wangweicugw Jan 31, 2024 •

edited

Loading

codecov bot commented Jan 30, 2024 •

edited

Loading

systay commented Jan 30, 2024 •

edited

Loading

wangweicugw commented Jan 31, 2024 •

edited

Loading

systay Jan 31, 2024

wangweicugw Jan 31, 2024

systay Jan 31, 2024

dbussink Feb 2, 2024

dbussink Feb 2, 2024

		ctx.SemTable.CopySemanticInfo(rae.Expr, col)
		ctx.SemTable.CopySemanticInfo(lae.Expr, col)

Copy expression types to avoid weight_strings and derived tables #15069

Copy expression types to avoid weight_strings and derived tables #15069

Conversation

wangweicugw commented Jan 29, 2024 • edited by systay Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Jan 29, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

This comment was marked as resolved.

This comment was marked as resolved.

systay Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

wangweicugw Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 30, 2024 • edited Loading

Codecov Report

systay commented Jan 30, 2024 • edited Loading

wangweicugw commented Jan 31, 2024 • edited Loading

systay Jan 31, 2024

Choose a reason for hiding this comment

wangweicugw Jan 31, 2024

Choose a reason for hiding this comment

systay Jan 31, 2024

Choose a reason for hiding this comment

dbussink Feb 2, 2024

Choose a reason for hiding this comment

dbussink Feb 2, 2024

Choose a reason for hiding this comment

wangweicugw commented Jan 29, 2024 •

edited by systay

Loading

systay Jan 30, 2024 •

edited

Loading

wangweicugw Jan 31, 2024 •

edited

Loading

codecov bot commented Jan 30, 2024 •

edited

Loading

systay commented Jan 30, 2024 •

edited

Loading

wangweicugw commented Jan 31, 2024 •

edited

Loading