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, type: fix AggFieldType error when encouter unsigned and sign type #21062

Conversation

rogeryk
Copy link
Contributor

@rogeryk rogeryk commented Nov 15, 2020

What problem does this PR solve?

Issue Number:
close #9869
close #17652

Problem Summary: not process UnsignedFlag in AggFieldType

What is changed and how it works?

What's Changed:

types/field_type.go

How it Works:

add UnsignedFlag check in mergeFlag
add integral promotion when encounter signed and unsigned

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM
  • Breaking backward compatibility

Release note

@rogeryk rogeryk requested review from a team as code owners November 15, 2020 11:37
@rogeryk rogeryk requested review from SunRunAway and removed request for a team November 15, 2020 11:37
@ti-srebot ti-srebot added the first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. label Nov 15, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Nov 15, 2020

@rogeryk
Copy link
Contributor Author

rogeryk commented Nov 15, 2020

/run-all-test

@rogeryk
Copy link
Contributor Author

rogeryk commented Nov 15, 2020

/rebuild

@ichn-hu ichn-hu mentioned this pull request Nov 15, 2020
@rogeryk rogeryk force-pushed the 9869-bigint-unsigned-in-casefunc-covert-to-signed branch from be2fcc3 to efc9e9f Compare November 15, 2020 15:33
@XuHuaiyu XuHuaiyu added the type/bugfix This PR fixes a bug. label Nov 16, 2020
@XuHuaiyu XuHuaiyu changed the title planner, type: fix AggFieldType error when encouter unsigned and sign type (#9869) planner, type: fix AggFieldType error when encouter unsigned and sign type Nov 16, 2020
@@ -28,17 +28,21 @@ import (

// NewOne stands for a number 1.
func NewOne() *Constant {
retT := types.NewFieldType(mysql.TypeTiny)
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not understand why we need to change these 2 funcs

Copy link
Contributor Author

@rogeryk rogeryk Nov 16, 2020

Choose a reason for hiding this comment

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

because i make test fail when i modify AggFieldType. The test is https://github.com/pingcap/tidb/blob/master/planner/cascades/transformation_rules_test.go#L389. "SQL": "select count(b), sum(b), avg(b), b, max(b), min(b), bit_and(b), bit_or(b), bit_xor(b) from t group by a having sum(b) >= 0 and count(b) >= 0 order by b",

The reason for fail is that the bit_or will add a cast wrap (cast(b as bigint unsigned binary), the arg is unsigned.
the TransformAggToProj will add ifnull wrap ifnull(cast(b as bigint unsigned binary), expression.NewZero()).
ifnull use AggFieldType infer type to tigger integral promotion.
https://github.com/pingcap/tidb/blob/master/planner/core/rule_aggregation_elimination.go#L124

I think this is a easy way to process. what do you think?

if isMixedSign && IsTypeInteger(currType.Tp) {
bumpRange := false // indicate one of tps bump currType range
for _, t := range tps {
bumpRange = bumpRange || (mysql.HasUnsignedFlag(t.Flag) && (t.Tp == currType.Tp || t.Tp == mysql.TypeBit))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why check mysql.TypeBit specially

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because the TypeBit can't be mergeType and assume it max range bit(64). so it must bump.
I find the all IntegerType merege mysql.TypeBit is mysql.TypeVarchar.
It is inconsistent with mysql 8.0 https://github.com/mysql/mysql-server/blob/8.0/sql/field.cc#L248.

Whether it will be consistent with mysql in the future. if not, No need to consider mysql.TypeBit here

currType.Tp = mtp
currType.Flag = mergeTypeFlag(currType.Flag, t.Flag)
}
// integral promotion when tps contains signed and unsigned
if isMixedSign && IsTypeInteger(currType.Tp) {
bumpRange := false // indicate one of tps bump currType range
Copy link
Contributor

Choose a reason for hiding this comment

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

Does MySQL also bumpRange ?

Copy link
Contributor Author

@rogeryk rogeryk Nov 16, 2020

Choose a reason for hiding this comment

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

Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 16, 2020
@XuHuaiyu XuHuaiyu requested review from lzmhhh123 and removed request for SunRunAway November 16, 2020 07:04
@XuHuaiyu
Copy link
Contributor

PTAL @lzmhhh123

@XuHuaiyu
Copy link
Contributor

@rogeryk Please add the test case in #17652 to this commit.
It seems that this PR can also fix that issue.

@rogeryk
Copy link
Contributor Author

rogeryk commented Nov 16, 2020

@rogeryk Please add the test case in #17652 to this commit.
It seems that this PR can also fix that issue.

Ok i will add

@rogeryk
Copy link
Contributor Author

rogeryk commented Nov 16, 2020

@rogeryk Please add the test case in #17652 to this commit.
It seems that this PR can also fix that issue.

done

@XuHuaiyu XuHuaiyu requested a review from ichn-hu November 17, 2020 08:02
@XuHuaiyu
Copy link
Contributor

PTAL @ichn-hu

ichn-hu
ichn-hu previously approved these changes Nov 19, 2020
Copy link
Contributor

@ichn-hu ichn-hu left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Nov 19, 2020
@rogeryk rogeryk force-pushed the 9869-bigint-unsigned-in-casefunc-covert-to-signed branch from 9fbdb62 to 4e5c29e Compare November 20, 2020 10:50
@rogeryk
Copy link
Contributor Author

rogeryk commented Nov 20, 2020

/run-all-tests

@rogeryk rogeryk force-pushed the 9869-bigint-unsigned-in-casefunc-covert-to-signed branch from 64c679b to 3306832 Compare November 20, 2020 17:39
@rogeryk
Copy link
Contributor Author

rogeryk commented Nov 20, 2020

/run-all-tests

@rogeryk rogeryk force-pushed the 9869-bigint-unsigned-in-casefunc-covert-to-signed branch from 3306832 to 4e5c29e Compare November 20, 2020 17:52
@rogeryk
Copy link
Contributor Author

rogeryk commented Nov 21, 2020

/run-unit-test

@rogeryk
Copy link
Contributor Author

rogeryk commented Nov 21, 2020

/run-all-tests

@rogeryk
Copy link
Contributor Author

rogeryk commented Nov 22, 2020

@lzmhhh123 The tests look not stable. How can I avoid it?

@lzmhhh123
Copy link
Contributor

@lzmhhh123 The tests look not stable. How can I avoid it?

Which test?

@rogeryk
Copy link
Contributor Author

rogeryk commented Nov 23, 2020

@lzmhhh123
Copy link
Contributor

What's the error of the fail.

@rogeryk
Copy link
Contributor Author

rogeryk commented Nov 23, 2020

What's the error of the fail.

The error is the estRows is not equal. now I have no infomation.

@lzmhhh123
Copy link
Contributor

Is this test failed?


[2020-11-23T09:45:42.107Z] physical_plan_test.go:1731:

[2020-11-23T09:45:42.107Z]     tk.MustQuery("explain " + ts).Check(testkit.Rows(output[i].Plan...))

[2020-11-23T09:45:42.107Z] /home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:63:

[2020-11-23T09:45:42.107Z]     res.c.Assert(resBuff.String(), check.Equals, needBuff.String(), res.comment)

[2020-11-23T09:45:42.107Z] ... obtained string = "" +

[2020-11-23T09:45:42.107Z] ...     "[IndexLookUp_7 1441.12 root  ]\n" +

[2020-11-23T09:45:42.107Z] ...     "[├─IndexRangeScan_5(Build) 1441.12 cop[tikv] table:test, index:idx_age(age) range:[5,5], keep order:false]\n" +

[2020-11-23T09:45:42.108Z] ...     "[└─TableRowIDScan_6(Probe) 1441.12 cop[tikv] table:test keep order:false]\n"

[2020-11-23T09:45:42.108Z] ... expected string = "" +

[2020-11-23T09:45:42.108Z] ...     "[IndexLookUp_7 2048.00 root  ]\n" +

[2020-11-23T09:45:42.108Z] ...     "[├─IndexRangeScan_5(Build) 2048.00 cop[tikv] table:test, index:idx_age(age) range:[5,5], keep order:false]\n" +

[2020-11-23T09:45:42.108Z] ...     "[└─TableRowIDScan_6(Probe) 2048.00 cop[tikv] table:test keep order:false]\n"

@rogeryk
Copy link
Contributor Author

rogeryk commented Nov 23, 2020

Is this test failed?


[2020-11-23T09:45:42.107Z] physical_plan_test.go:1731:

[2020-11-23T09:45:42.107Z]     tk.MustQuery("explain " + ts).Check(testkit.Rows(output[i].Plan...))

[2020-11-23T09:45:42.107Z] /home/jenkins/agent/workspace/tidb_ghpr_check_2/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:63:

[2020-11-23T09:45:42.107Z]     res.c.Assert(resBuff.String(), check.Equals, needBuff.String(), res.comment)

[2020-11-23T09:45:42.107Z] ... obtained string = "" +

[2020-11-23T09:45:42.107Z] ...     "[IndexLookUp_7 1441.12 root  ]\n" +

[2020-11-23T09:45:42.107Z] ...     "[├─IndexRangeScan_5(Build) 1441.12 cop[tikv] table:test, index:idx_age(age) range:[5,5], keep order:false]\n" +

[2020-11-23T09:45:42.108Z] ...     "[└─TableRowIDScan_6(Probe) 1441.12 cop[tikv] table:test keep order:false]\n"

[2020-11-23T09:45:42.108Z] ... expected string = "" +

[2020-11-23T09:45:42.108Z] ...     "[IndexLookUp_7 2048.00 root  ]\n" +

[2020-11-23T09:45:42.108Z] ...     "[├─IndexRangeScan_5(Build) 2048.00 cop[tikv] table:test, index:idx_age(age) range:[5,5], keep order:false]\n" +

[2020-11-23T09:45:42.108Z] ...     "[└─TableRowIDScan_6(Probe) 2048.00 cop[tikv] table:test keep order:false]\n"

yes, sometime it is right.

@XuHuaiyu
Copy link
Contributor

#21092

It has nothing to do with this PR. I'll trigger merge again.

@XuHuaiyu
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 21124

@ti-srebot
Copy link
Contributor

/run-all-tests

@XuHuaiyu XuHuaiyu merged commit 4897ecd into pingcap:master Nov 24, 2020
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Nov 24, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #21236

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
6 participants