Bugfix: GROUP BY/HAVING alias resolution #15344

systay · 2024-02-23T14:34:31Z

Description

Following up on #15302, this PR does the same fix for GROUP BY and HAVING.

Related Issue(s)

#15302 (fix for ORDER BY)
#14935 (pr that introduced some bad rewrites)

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

vitess-bot · 2024-02-23T14:34:34Z

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

go/vt/vtgate/planbuilder/testdata/aggr_cases.json

systay · 2024-02-26T16:18:04Z

go/vt/vtgate/planbuilder/testdata/aggr_cases.json

@@ -1041,32 +1073,6 @@
      ]
    }
  },
-  {


Removed this since it fails on mysql. a is not the same as a collate utf8_general_ci, so if full_group_by is enabled, this query would fail

systay · 2024-02-26T16:19:02Z

go/vt/vtgate/planbuilder/testdata/aggr_cases.json

@@ -2113,71 +2119,6 @@
      ]
    }
  },
-  {
-    "comment": "aggregation filtering by having on a route with no group by with non-unique vindex filter",


this query fails on both mysql and vitess. name is not accessible on the having clause

This query does not fail anymore, we should bring the test back.

codecov · 2024-02-26T16:34:24Z

Codecov Report

Attention: Patch coverage is 88.25758% with 31 lines in your changes are missing coverage. Please review.

Project coverage is 65.43%. Comparing base (696fe0e) to head (9eb1dc8).
Report is 65 commits behind head on main.

Files	Patch %	Lines
go/vt/vtgate/semantics/binder.go	86.04%	12 Missing ⚠️
go/vt/vtgate/semantics/errors.go	33.33%	10 Missing ⚠️
go/vt/vtgate/semantics/early_rewriter.go	95.68%	5 Missing ⚠️
go/vt/vtgate/semantics/analyzer.go	85.71%	2 Missing ⚠️
go/vt/vtgate/semantics/dependencies.go	66.66%	1 Missing ⚠️
go/vt/vtgate/semantics/scoper.go	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15344      +/-   ##
==========================================
- Coverage   67.41%   65.43%   -1.98%     
==========================================
  Files        1560     1562       +2     
  Lines      192752   193863    +1111     
==========================================
- Hits       129952   126863    -3089     
- Misses      62800    67000    +4200

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

GuptaManan100

Rest LGTM

go/vt/vtgate/semantics/early_rewriter.go

systay · 2024-02-27T15:08:06Z

does this mean we will start failing queries for group by/ having if the schema tracking is not enabled?

your comment was not connected to anything I could see, so I'm not sure what the question is about. Generally - if you are grouping by an alias introduced in the SELECT expression, and we have to do some or most of the aggregating on the vtgate, queries might start having issues if you are running without schema tracking

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

…ER BY/HAVING clause Signed-off-by: Andres Taylor <[email protected]>

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

go/vt/vtgate/semantics/binder.go

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

go/vt/vtgate/planbuilder/testdata/aggr_cases.json

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

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

Signed-off-by: Andres Taylor <[email protected]> Signed-off-by: Manan Gupta <[email protected]> Co-authored-by: Manan Gupta <[email protected]>

) Signed-off-by: Andres Taylor <[email protected]> Signed-off-by: Manan Gupta <[email protected]> Co-authored-by: Andrés Taylor <[email protected]> Co-authored-by: Manan Gupta <[email protected]>

* fix test cases * Merge branch 'master' into split-table-merge-master0306 * fix ci test * Bugfix: GROUP BY/HAVING alias resolution (vitessio#15344) (vitessio#15381 * add params for proxy support ddl operations * merge origin * fix client_found_row (vitessio#203) * merge origin to a2c986c * fix client found rows * Use GetTabletsByCell in healthcheck * delete start params:track_schema_versions. * auto delete logs from three days ago * 解决跨cell获取节点失败和SQL权限没有正确拦截的问题 * merge origin 3a35dd * 支持MySQL8.0编译... (vitessio#185) * 修改show vitess_shards|vitess_tables和 show variables (vitessio#175) * fix binaryhash * fix: container restart ，programs restart failed * add start params * merge origin with laster pr:f39ada * fix vttablet startup * fix openEuler dockerfile * merge origin bugfix * fix master ci * Bugfix: fix pinned table for splitclone (vitessio#141)

* Merge branch 'master' into split-table-merge-master0306 * 修复agent容器无法跨容器恢复MySQL数据的问题 * fix test cases * fix ci test * Bugfix: GROUP BY/HAVING alias resolution (vitessio#15344) (vitessio#15381 * add params for proxy support ddl operations * merge origin * fix client_found_row (vitessio#203) * merge origin to a2c986c * fix client found rows * Use GetTabletsByCell in healthcheck * delete start params:track_schema_versions. * auto delete logs from three days ago * 解决跨cell获取节点失败和SQL权限没有正确拦截的问题 * merge origin 3a35dd * 支持MySQL8.0编译... (vitessio#185) * 修改show vitess_shards|vitess_tables和 show variables (vitessio#175) * fix binaryhash * fix: container restart ，programs restart failed * add start params * merge origin with laster pr:f39ada * fix vttablet startup * fix openEuler dockerfile * merge origin bugfix * fix master ci * Bugfix: fix pinned table for splitclone (vitessio#141)

github-actions bot added this to the v20.0.0 milestone Feb 23, 2024

systay force-pushed the group-by-resolution branch from 44dca2d to 7a0834e Compare February 23, 2024 14:47

systay added Type: Bug Component: Query Serving labels Feb 26, 2024

systay added 5 commits February 26, 2024 08:43

wip - fixed group by and added tests for HAVING

ecef290

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

wip - solve the semantic side of HAVING

316de79

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

test: add HAVING end to end tests

e5fd302

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

test: clean up state from all tables

9a09eb4

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

test: allow failures as long as mysql also fails

032275f

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

systay force-pushed the group-by-resolution branch from a74875d to 032275f Compare February 26, 2024 07:43

test: update test expectations

1f8838f

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

systay force-pushed the group-by-resolution branch from 689f3c2 to 1f8838f Compare February 26, 2024 16:15

systay commented Feb 26, 2024

View reviewed changes

go/vt/vtgate/planbuilder/testdata/aggr_cases.json Show resolved Hide resolved

systay commented Feb 26, 2024

View reviewed changes

systay marked this pull request as ready for review February 27, 2024 06:51

systay requested review from harshit-gangal and frouioui as code owners February 27, 2024 06:51

GuptaManan100 approved these changes Feb 27, 2024

View reviewed changes

go/vt/vtgate/semantics/early_rewriter.go Outdated Show resolved Hide resolved

frouioui approved these changes Feb 27, 2024

View reviewed changes

systay added 4 commits February 28, 2024 09:05

feat: make HAVING and ORDER BY behave more like MySQL

b11a753

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

feat: simplify logic and add warnings

0afc4d0

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

feat: when schema info is missing, don't bind column names in the ORD…

99dfa75

…ER BY/HAVING clause Signed-off-by: Andres Taylor <[email protected]>

test: update expectations

2412b9c

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

systay mentioned this pull request Feb 28, 2024

[release-19.0] Bugfix: GROUP BY/HAVING alias resolution (#15344) #15377

Merged

GuptaManan100 approved these changes Feb 28, 2024

View reviewed changes

harshit-gangal reviewed Feb 28, 2024

View reviewed changes

go/vt/vtgate/semantics/binder.go Outdated Show resolved Hide resolved

feat: don't allow HAVING to use FROM clause outside of aggregations

2b0596b

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

harshit-gangal reviewed Feb 28, 2024

View reviewed changes

go/vt/vtgate/planbuilder/testdata/aggr_cases.json Show resolved Hide resolved

systay added 3 commits February 28, 2024 14:05

feat: stop HAVING from looking at the FROM clause in the precense of …

7f1a9d1

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

feat: allow column names that are used as a SELECT expression

5b169dd

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

feat: Allow HAVING to see expressions in the GROUP BY

70348a3

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

systay force-pushed the group-by-resolution branch from 56e4696 to 70348a3 Compare February 28, 2024 15:39

test: return deleted tests

9eb1dc8

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

systay removed the Backport to: release-19.0 Needs to be back ported to release-19.0 label Feb 28, 2024

systay merged commit 15f5886 into vitessio:main Feb 28, 2024
103 checks passed

systay deleted the group-by-resolution branch February 28, 2024 20:52

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

[release-18.0] Bugfix: GROUP BY/HAVING alias resolution (#15344) #15381

Merged

systay added a commit to planetscale/vitess that referenced this pull request Feb 29, 2024

This is a backport of vitessio#15344

f2d010a

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

systay added a commit that referenced this pull request Feb 29, 2024

Bugfix: GROUP BY/HAVING alias resolution (#15344)

01d460d

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

GuptaManan100 added a commit that referenced this pull request Feb 29, 2024

This is a backport of #15344 (#15377)

977b585

Signed-off-by: Andres Taylor <[email protected]> Signed-off-by: Manan Gupta <[email protected]> Co-authored-by: Manan Gupta <[email protected]>

arthurschreiber mentioned this pull request Oct 15, 2024

Bug Report: Vitess generates order by / group by ambiguity warnings where MySQL does not #16971

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: GROUP BY/HAVING alias resolution #15344

Bugfix: GROUP BY/HAVING alias resolution #15344

systay commented Feb 23, 2024 •

edited

Loading

vitess-bot bot commented Feb 23, 2024

systay Feb 26, 2024

systay Feb 26, 2024

harshit-gangal Feb 28, 2024

codecov bot commented Feb 26, 2024 •

edited

Loading

GuptaManan100 left a comment

systay commented Feb 27, 2024

@@ @@ -1041,32 +1073,6 @@ @@
                     ]
                   }
                 },
-                {

Bugfix: GROUP BY/HAVING alias resolution #15344

Bugfix: GROUP BY/HAVING alias resolution #15344

Conversation

systay commented Feb 23, 2024 • edited Loading

Description

Related Issue(s)

Checklist

vitess-bot bot commented Feb 23, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

systay Feb 26, 2024

Choose a reason for hiding this comment

systay Feb 26, 2024

Choose a reason for hiding this comment

harshit-gangal Feb 28, 2024

Choose a reason for hiding this comment

codecov bot commented Feb 26, 2024 • edited Loading

Codecov Report

GuptaManan100 left a comment

Choose a reason for hiding this comment

systay commented Feb 27, 2024

systay commented Feb 23, 2024 •

edited

Loading

codecov bot commented Feb 26, 2024 •

edited

Loading