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.

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

planner: fix bug for logical rule outer join elimination #13947

Merged
merged 2 commits into from
Dec 10, 2019

Conversation

lzmhhh123
Copy link
Contributor

What problem does this PR solve?

CREATE TABLE `test01` (
`id` bigint(20) NOT NULL AUTO_INCREMENT,
`stat_date` int(11) NOT NULL DEFAULT '0',
`show_date` varchar(20) NOT NULL DEFAULT '',
`region_id` bigint(20) unsigned NOT NULL DEFAULT '0',
`period` tinyint(3) unsigned NOT NULL DEFAULT '0',
`registration_num` bigint(20) unsigned NOT NULL DEFAULT '0',
PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin;

CREATE TABLE `test02` (
`id` bigint(20) NOT NULL AUTO_INCREMENT,
`region_name` varchar(128) DEFAULT NULL,
PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin;

SELECT
COUNT(1)
FROM
(SELECT
COALESCE(b.region_name, '不详') region_name,
SUM(a.registration_num) registration_num
FROM
(SELECT
stat_date,
show_date,
region_id,
0 registration_num
FROM
test01
WHERE
period = 1 AND stat_date >= 20191202
AND stat_date <= 20191202 UNION ALL SELECT
stat_date,
show_date,
region_id,
registration_num registration_num
FROM
test01
WHERE
period = 1 AND stat_date >= 20191202
AND stat_date <= 20191202) a
LEFT JOIN test02 b ON a.region_id = b.id
WHERE
registration_num > 0
AND a.stat_date >= '20191202'
AND a.stat_date <= '20191202'
GROUP BY a.stat_date , a.show_date , COALESCE(b.region_name, '不详')
) JLS;

ERROR 1105 (HY000): Can't find column ryltest.b.region_name in schema Column: [a.stat_date,a.show_date,a.region_id,a.registration_num] Unique key: []

What is changed and how it works?

The aggregation group by columns shouldn't be groupByCols, instead of it, it should be expression.ExtractColumns(groupByItems).

Check List

Tests

  • Unit test
  • Integration test

@lzmhhh123 lzmhhh123 added type/bugfix This PR fixes a bug. sig/planner SIG: Planner needs-cherry-pick-3.0 labels Dec 6, 2019
@lzmhhh123 lzmhhh123 requested a review from a team as a code owner December 6, 2019 08:27
@ghost ghost requested review from eurekaka and francis0407 and removed request for a team December 6, 2019 08:27
@lzmhhh123
Copy link
Contributor Author

/run-all-tests

@codecov
Copy link

codecov bot commented Dec 6, 2019

Codecov Report

Merging #13947 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #13947   +/-   ##
===========================================
  Coverage   80.2047%   80.2047%           
===========================================
  Files           482        482           
  Lines        120741     120741           
===========================================
  Hits          96840      96840           
  Misses        16180      16180           
  Partials       7721       7721

Copy link
Member

@francis0407 francis0407 left a comment

Choose a reason for hiding this comment

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

LGTM

@francis0407 francis0407 added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 6, 2019
Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -199,7 +199,10 @@ func (o *outerJoinEliminator) doOptimize(p LogicalPlan, aggCols []*expression.Co
parentCols = append(parentCols, expression.ExtractColumns(expr)...)
}
case *LogicalAggregation:
parentCols = append(parentCols[:0], x.groupByCols...)
Copy link
Member

Choose a reason for hiding this comment

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

A question won't block merging this pr: could we just remove the groupByCols from the Aggregation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can't. groupByCols are used to check whether aggregation can keep order when generating stream agg.

@winoros
Copy link
Member

winoros commented Dec 10, 2019

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 10, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 10, 2019

/run-all-tests

@sre-bot sre-bot merged commit c796205 into pingcap:master Dec 10, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 10, 2019

cherry pick to release-3.0 failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants