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

expression: fix different types compare error #21338

Merged
merged 24 commits into from
Dec 25, 2020

Conversation

zhaoxugang
Copy link
Contributor

@zhaoxugang zhaoxugang commented Nov 27, 2020

What problem does this PR solve?

Issue Number: close #20128

Problem Summary:

What is changed and how it works?

What's Changed:
fix different types compare error
How it Works:

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)
  • No code

Release note

  • fix different types compare error

@zhaoxugang zhaoxugang requested a review from a team as a code owner November 27, 2020 00:03
@zhaoxugang zhaoxugang requested review from XuHuaiyu and removed request for a team November 27, 2020 00:03
@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 27, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Nov 27, 2020

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 28, 2020

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 28, 2020

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 28, 2020

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 29, 2020

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@sre-bot
Copy link
Contributor

sre-bot commented Dec 1, 2020

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@sre-bot
Copy link
Contributor

sre-bot commented Dec 1, 2020

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@zhaoxugang zhaoxugang changed the title fix different types compare error and query for bit expression: fix different types compare error and query for bit Dec 1, 2020
@zhaoxugang
Copy link
Contributor Author

/run-all-tests

@zhaoxugang zhaoxugang changed the title expression: fix different types compare error and query for bit expression: fix different types compare error and format result of query for bit Dec 1, 2020
@zhaoxugang
Copy link
Contributor Author

/run-all-tests

@zhaoxugang zhaoxugang changed the title expression: fix different types compare error and format result of query for bit expression:fix different types compare error and format result of query for bit Dec 1, 2020
@zhaoxugang zhaoxugang changed the title expression:fix different types compare error and format result of query for bit expression: fix different types compare error and format result of query for bit Dec 1, 2020
@zhaoxugang
Copy link
Contributor Author

/rebuild

@zhaoxugang
Copy link
Contributor Author

/run-all-tests

1 similar comment
@zhaoxugang
Copy link
Contributor Author

/run-all-tests

@XuHuaiyu
Copy link
Contributor

PTAL @wshwsh12

if fun, ok := res.(*ScalarFunction); (ok && fun.FuncName.L == ast.Cast) || res.GetType().Tp == mysql.TypeBit {
if evalType == types.ETInt || evalType == types.ETReal {
lfg, rfg := args[0].GetType().Flag, args[1].GetType().Flag
// set signedFlag to 0 when bit convert explicitly or implicitly to int or real
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't understand the logic. In the case, all evalType.flag don't contain UnsignedFlag, I think. Could you provide an example?

drop table if exists t;
create table t(a bit(64), b double);
insert into t values(-21172, -11623);
select * from t where a < b;

I have a try, only add builtin_cast.goL487 and builtin_cast_vec.goL111, it also works. Is there any negligence in what I thought?

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 removed builtin.goL173,it not work,for this case i see the step:
1.Invoke GetAccurateCmpType() output ETReal
2.Invoke newBaseBuiltinFuncWithTp()
in this method the param a will produce the scalarFunction named cast,retFlag=160,arg's retFlag=32 by WrapWithCastAsReal()
so in builtin_cast_vec.goL111 the unsignedArgs0=true,the a will change to uint.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why the flag for column a is 32..? It doesn't has the unsigned flag when creating table.

Copy link
Contributor

Choose a reason for hiding this comment

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

If cast a as double explicitly, it works...

[tidb]>  select * from t where cast(a as double) < b;
+----------+--------+
| a        | b      |
+----------+--------+
| �������L        | -11623 |
+----------+--------+
1 row in set (5.925 sec)

Copy link
Contributor Author

@zhaoxugang zhaoxugang Dec 20, 2020

Choose a reason for hiding this comment

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

in ddl_api.goL537 invoke col.Flag |= mysql.UnsignedFlag when create table,
you mean i should't change retFlag in generateRetFlag, but cast bit to double?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a try in mysql8.0.22.. So the behavior in tidb is right? It maybe a bug in mysql8.0.21 and the previous version?

MySQL [test]> select version();
+-----------+
| version() |
+-----------+
| 8.0.22    |
+-----------+
1 row in set (0.000 sec)

MySQL [test]> drop table if exists t;
Query OK, 0 rows affected (0.006 sec)

MySQL [test]> create table t(a bit(64), b double);
Query OK, 0 rows affected (0.009 sec)

MySQL [test]> insert into t values(-21172, -11623);
Query OK, 1 row affected (0.002 sec)

MySQL [test]> select * from t where a < b;
Empty set (0.000 sec)

But in mysql8.0.21,

MySQL [test]> select version();
+-----------+
| version() |
+-----------+
| 8.0.21    |
+-----------+
1 row in set (0.000 sec)

MySQL [test]> drop table if exists t;
Query OK, 0 rows affected (0.007 sec)

MySQL [test]> create table t(a bit(64), b double);
Query OK, 0 rows affected (0.008 sec)

MySQL [test]> insert into t values(-21172, -11623);
Query OK, 1 row affected (0.001 sec)

MySQL [test]> select * from t where a < b;
+----------+--------+
| a        | b      |
+----------+--------+
| �������L        | -11623 |
+----------+--------+
1 row in set (0.000 sec)

expression/builtin.go Outdated Show resolved Hide resolved
@zhaoxugang
Copy link
Contributor Author

/run-all-test

@wshwsh12 wshwsh12 self-requested a review December 20, 2020 06:48
expression/builtin.go Outdated Show resolved Hide resolved
Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

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

Rest LGTM

expression/integration_test.go Outdated Show resolved Hide resolved
@wshwsh12
Copy link
Contributor

Please resolve conflicts.

Copy link
Contributor

@wshwsh12 wshwsh12 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 Dec 24, 2020
Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

Please resolve conflicts.

Copy link
Contributor

@lzmhhh123 lzmhhh123 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 Dec 25, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 25, 2020
@lzmhhh123
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 25, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 3e2ff1d into pingcap:master Dec 25, 2020
tangenta pushed a commit to tangenta/tidb that referenced this pull request Dec 28, 2020
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. sig/sql-infra SIG: SQL Infra 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
Development

Successfully merging this pull request may close these issues.

different types compare error
7 participants