bugfix: Normalizing literals in SELECT is broken #15043

systay · 2024-01-26T07:38:11Z

Description

Vitess does auto-parameterization of queries. This means replacing literal values with arguments, so that a plan cache can be re-used between queries, if the only difference is the literal value.

Sometimes, this rewrite messes with queries, such as in the issue reported below.

In this PR, I wanted to stop the Normalizer from rewriting expressions if they are inside unalised select expressions.

To do that, first I needed to clean up our AST a little bit.

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.

Related Issue(s)

Fixes #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

vitess-bot · 2024-01-26T07:38:14Z

systay · 2024-01-26T08:19:37Z

go/vt/sqlparser/ast.go

@@ -2398,7 +2398,7 @@ type (
 	FuncExpr struct {
 		Qualifier IdentifierCS
 		Name      IdentifierCI
-		Exprs     SelectExprs


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.

Love this change

harshit-gangal · 2024-01-30T12:44:26Z

go/test/endtoend/vtgate/queries/subquery/subquery_test.go

@@ -170,7 +170,7 @@ 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`)
+	mcmp.Exec(`SELECT max((select group_concat(id1, id2) from t1 where id1 = 1)) as x FROM t1 where id1 = 1`)


why an alias is needed here?

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

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

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

go/vt/sqlparser/normalizer.go

Signed-off-by: Andres Taylor <[email protected]>

GrahamCampbell · 2024-02-06T13:11:46Z

Needs a backport to 19.0 label?

Signed-off-by: Andres Taylor <[email protected]>

systay · 2024-02-06T13:27:28Z

Needs a backport to 19.0 label?

Not sure we want to backport this. @deepthi @harshit-gangal wdyt?

codecov · 2024-02-06T13:45:14Z

Codecov Report

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

Comparison is base (4e31f60) 67.29% compared to head (08cabf1) 67.27%.

Files	Patch %	Lines
go/vt/vtgate/planbuilder/operators/aggregator.go	0.00%	5 Missing ⚠️
go/vt/vttablet/tabletmanager/vdiff/table_differ.go	0.00%	4 Missing ⚠️
go/vt/vtgate/simplifier/expression_simplifier.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15043      +/-   ##
==========================================
- Coverage   67.29%   67.27%   -0.02%     
==========================================
  Files        1560     1560              
  Lines      192123   192109      -14     
==========================================
- Hits       129283   129249      -34     
- Misses      62840    62860      +20

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

GuptaManan100

Rest LGTM!

go/test/endtoend/vtgate/queries/collations/collations_test.go

go/test/endtoend/vtgate/queries/collations/main_test.go

Signed-off-by: Andres Taylor <[email protected]>

frouioui · 2024-02-07T17:06:46Z

Not sure we want to backport this. @deepthi @harshit-gangal wdyt?

I feel like we should given this is a bug fix, but i have no strong opinions.

EDIT: I think we should also backport to release-18.0, the linked issue (#15020) was using v18.0.1.

frouioui

I think it's a good change. Using sqlparser.Expr instead of sqlparser.SelectExpr interface is awesome

go/test/endtoend/vtgate/queries/collations/collations_test.go

harshit-gangal · 2024-02-08T08:10:50Z

Needs a backport to 19.0 label?

Not sure we want to backport this. @deepthi @harshit-gangal wdyt?

some part of it is regression from v17 and some are new support. So, we should backport it till v18

harshit-gangal

Approving with 1 comment about adding an unsupported query test.

Signed-off-by: Andres Taylor <[email protected]>

harshit-gangal

As discussed offline, this will cause regression for aggregation subquery without an alias.

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

systay commented Jan 26, 2024

View reviewed changes

systay changed the title ~~bugfix: Literals in SELECT are broken~~ bugfix: Normalizing literals in SELECT is broken Jan 26, 2024

This comment was marked as outdated.

Sign in to view

systay force-pushed the literal-aliased-expr branch from 1bd8dae to 2a18da2 Compare January 30, 2024 07:38

systay marked this pull request as ready for review January 30, 2024 07:39

systay requested review from deepthi, mattlord, rohit-nayak-ps, harshit-gangal, shlomi-noach, GuptaManan100, frouioui, arthurschreiber and ajm188 as code owners January 30, 2024 07:39

harshit-gangal reviewed Jan 30, 2024

View reviewed changes

go/vt/sqlparser/normalizer.go Outdated Show resolved Hide resolved

systay modified the milestones: v19.0.0, v20.0.0 Feb 6, 2024

systay added 2 commits February 6, 2024 13:39

test: add end to end test for UNION collation handling

dfe8e6e

Signed-off-by: Andres Taylor <[email protected]>

test: update test expectations

3c92aed

Signed-off-by: Andres Taylor <[email protected]>

systay force-pushed the literal-aliased-expr branch from de8fc91 to 3c92aed Compare February 6, 2024 12:55

systay requested a review from harshit-gangal February 6, 2024 12:55

test: remove outdated tests

767bcd5

Signed-off-by: Andres Taylor <[email protected]>

GuptaManan100 approved these changes Feb 7, 2024

View reviewed changes

go/test/endtoend/vtgate/queries/collations/collations_test.go Outdated Show resolved Hide resolved

go/test/endtoend/vtgate/queries/collations/main_test.go Outdated Show resolved Hide resolved

systay force-pushed the literal-aliased-expr branch from fd38aab to e831863 Compare February 7, 2024 15:27

systay requested a review from notfelineit as a code owner February 7, 2024 15:27

systay added 2 commits February 7, 2024 16:27

Merge remote-tracking branch 'upstream/main' into literal-aliased-expr

1ca5a3d

Signed-off-by: Andres Taylor <[email protected]>

update license year

96c3ed3

Signed-off-by: Andres Taylor <[email protected]>

systay force-pushed the literal-aliased-expr branch from e831863 to 96c3ed3 Compare February 7, 2024 15:28

frouioui approved these changes Feb 7, 2024

View reviewed changes

go/test/endtoend/vtgate/queries/collations/collations_test.go Outdated Show resolved Hide resolved

harshit-gangal added Backport to: release-18.0 Backport to: release-19.0 Needs to be back ported to release-19.0 labels Feb 8, 2024

harshit-gangal approved these changes Feb 8, 2024

View reviewed changes

systay added 2 commits February 9, 2024 13:30

test: clean out debug code

210e641

Signed-off-by: Andres Taylor <[email protected]>

Merge remote-tracking branch 'upstream/main' into literal-aliased-expr

08cabf1

Signed-off-by: Andres Taylor <[email protected]>

harshit-gangal requested changes Feb 12, 2024

View reviewed changes

systay mentioned this pull request Feb 26, 2024

Release of v19.0.0 #15357

Closed

38 tasks

systay marked this pull request as draft February 26, 2024 10:51

This was referenced Mar 13, 2024

Release of v18.0.3 #15468

Closed

Release of v19.0.1 #15469

Closed

frouioui removed Backport to: release-18.0 Backport to: release-19.0 Needs to be back ported to release-19.0 labels Mar 13, 2024

systay closed this Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix: Normalizing literals in SELECT is broken #15043

bugfix: Normalizing literals in SELECT is broken #15043

systay commented Jan 26, 2024 •

edited

Loading

vitess-bot bot commented Jan 26, 2024

systay Jan 26, 2024

GuptaManan100 Feb 7, 2024

This comment was marked as outdated.

harshit-gangal Jan 30, 2024

systay Feb 6, 2024

harshit-gangal Feb 7, 2024

systay Feb 9, 2024

GrahamCampbell commented Feb 6, 2024

systay commented Feb 6, 2024

codecov bot commented Feb 6, 2024 •

edited

Loading

GuptaManan100 left a comment

frouioui commented Feb 7, 2024 •

edited

Loading

frouioui left a comment

harshit-gangal commented Feb 8, 2024

harshit-gangal left a comment •

edited

Loading

harshit-gangal left a comment

bugfix: Normalizing literals in SELECT is broken #15043

bugfix: Normalizing literals in SELECT is broken #15043

Conversation

systay commented Jan 26, 2024 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Jan 26, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

systay Jan 26, 2024

Choose a reason for hiding this comment

GuptaManan100 Feb 7, 2024

Choose a reason for hiding this comment

This comment was marked as outdated.

harshit-gangal Jan 30, 2024

Choose a reason for hiding this comment

systay Feb 6, 2024

Choose a reason for hiding this comment

harshit-gangal Feb 7, 2024

Choose a reason for hiding this comment

systay Feb 9, 2024

Choose a reason for hiding this comment

GrahamCampbell commented Feb 6, 2024

systay commented Feb 6, 2024

codecov bot commented Feb 6, 2024 • edited Loading

Codecov Report

GuptaManan100 left a comment

Choose a reason for hiding this comment

frouioui commented Feb 7, 2024 • edited Loading

frouioui left a comment

Choose a reason for hiding this comment

harshit-gangal commented Feb 8, 2024

harshit-gangal left a comment • edited Loading

Choose a reason for hiding this comment

harshit-gangal left a comment

Choose a reason for hiding this comment

systay commented Jan 26, 2024 •

edited

Loading

codecov bot commented Feb 6, 2024 •

edited

Loading

frouioui commented Feb 7, 2024 •

edited

Loading

harshit-gangal left a comment •

edited

Loading