Skip to content

*: let baseFuncDesc.typeInfer return error instead of panic#10910

Merged
qw4990 merged 2 commits intopingcap:masterfrom
XuHuaiyu:agg_panic_to_err
Jun 24, 2019
Merged

*: let baseFuncDesc.typeInfer return error instead of panic#10910
qw4990 merged 2 commits intopingcap:masterfrom
XuHuaiyu:agg_panic_to_err

Conversation

@XuHuaiyu
Copy link
Contributor

What problem does this PR solve?

Before this commit, the following sql will make tidb-server panic.

tidb> create table t(a int);
Query OK, 0 rows affected (0.02 sec)

tidb> select std(a) from t;
ERROR 2013 (HY000): Lost connection to MySQL server during query

Afer this commit,

tidb> select std(a) from t;
ERROR 1105 (HY000): unsupported agg function: std

What is changed and how it works?

Let baseFuncDesc.typeInfer return error instead of panic.

Check List

Tests

  • Integration test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change

Side effects

N/A

Related changes

  • Need to cherry-pick to the release branch

@XuHuaiyu XuHuaiyu added type/bugfix This PR fixes a bug. sig/planner SIG: Planner labels Jun 23, 2019
@shenli
Copy link
Member

shenli commented Jun 23, 2019

Good job!

@codecov
Copy link

codecov bot commented Jun 23, 2019

Codecov Report

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

@@            Coverage Diff            @@
##            master    #10910   +/-   ##
=========================================
  Coverage   81.359%   81.359%           
=========================================
  Files          419       419           
  Lines        90119     90119           
=========================================
  Hits         73320     73320           
  Misses       11580     11580           
  Partials      5219      5219

@XuHuaiyu
Copy link
Contributor Author

The explain test changed because of this line:
https://github.com/pingcap/tidb/pull/10910/files#diff-1a802e13255ca722504fe1b0cc5e34abR321

This line should be p = a.aggPushDown(p).

@XuHuaiyu
Copy link
Contributor Author

@XuHuaiyu
Copy link
Contributor Author

/run-all-tests

@XuHuaiyu
Copy link
Contributor Author

/run-sqllogic-test-1

@mahjonp
Copy link
Contributor

mahjonp commented Jun 23, 2019

/run-sqllogic-test-1

XuHuaiyu added a commit to XuHuaiyu/tidb that referenced this pull request Jun 23, 2019
XuHuaiyu added a commit to XuHuaiyu/tidb that referenced this pull request Jun 23, 2019
@XuHuaiyu XuHuaiyu added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Jun 24, 2019
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

└─IndexReader_19 8000.00 root index:StreamAgg_10
└─StreamAgg_10 8000.00 cop group by:test.t1.a, test.t1.b, funcs:firstrow(test.t1.a), firstrow(test.t1.b)
└─IndexScan_17 10000.00 cop table:t1, index:a, b, range:[NULL,+inf], keep order:true, stats:pseudo
TableReader_9 10000.00 root data:TableScan_8
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this plan was changed?

Copy link
Contributor Author

@XuHuaiyu XuHuaiyu Jun 24, 2019

Choose a reason for hiding this comment

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

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 24, 2019
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

@qw4990 qw4990 merged commit c59028a into pingcap:master Jun 24, 2019
XuHuaiyu added a commit to XuHuaiyu/tidb that referenced this pull request Jun 24, 2019
@XuHuaiyu XuHuaiyu deleted the agg_panic_to_err branch July 4, 2019 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority/release-blocker This issue blocks a release. Please solve it ASAP. sig/planner SIG: Planner 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.

6 participants