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

executor: fix bug about JSON_EXTRACT fails to cast as bool #12440

Closed
wants to merge 11 commits into from
Closed

executor: fix bug about JSON_EXTRACT fails to cast as bool #12440

wants to merge 11 commits into from

Conversation

tsthght
Copy link
Contributor

@tsthght tsthght commented Sep 27, 2019

What problem does this PR solve?

fix bug about JSON_EXTRACT fails to cast as bool #12233
@ngaut

What is changed and how it works?

create database testjson;
use testjson;
create table testjson( id int auto_increment not null primary key, j json )default charset=utf8 engine=innodb;
insert into testjson set j='{"test":1}';
select id from testjson where json_extract(j, '$.test');
+----+
| id |
+----+
|  1 |
+----+
1 row in set (0.001 sec)

Check List

Tests

  • Unit test

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Sep 27, 2019
@codecov
Copy link

codecov bot commented Sep 27, 2019

Codecov Report

Merging #12440 into master will decrease coverage by 0.3721%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##             master    #12440        +/-   ##
===============================================
- Coverage   80.3452%   79.973%   -0.3722%     
===============================================
  Files           465       465                
  Lines        108396    106257      -2139     
===============================================
- Hits          87091     84977      -2114     
+ Misses        14952     14944         -8     
+ Partials       6353      6336        -17

Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, I've left some comments, PTAL

@@ -1390,6 +1390,8 @@ func (d *Datum) ToBool(sc *stmtctx.StatementContext) (int64, error) {
case KindBinaryLiteral, KindMysqlBit:
val, err1 := d.GetBinaryLiteral().ToInt(sc)
isZero, err = val == 0, err1
case KindMysqlJSON:
isZero, err = d.IsNull(), nil
Copy link
Contributor

@SunRunAway SunRunAway Sep 29, 2019

Choose a reason for hiding this comment

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

It should be false if $.test is zero.
Please add a test case like {"test":0} to cover it.

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 had add testcase as follows:
a) testcase1:
insert into testjson set j='{"test":0}
select j from testjson where json_extract(j, '$.test')

b) testcase2:
insert into testjson set j='{"test1":0}
select j from testjson where json_extract(j, '$.test')

Copy link
Contributor

Choose a reason for hiding this comment

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

I've tested this case in Mysql5.7, and the result is different from yours.

mysql> drop table testjson;
Query OK, 0 rows affected (0.00 sec)

mysql> create table testjson(j json );
Query OK, 0 rows affected (0.01 sec)

mysql> insert into testjson set j='{"test":0}';
Query OK, 1 row affected (0.00 sec)

mysql> select j from testjson where json_extract(j, '$.test');
Empty set (0.00 sec)

Copy link
Contributor

Choose a reason for hiding this comment

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

Another case is different from Mysql5.7

In your branch:

mysql> create table testjson(j json);
Query OK, 0 rows affected (0.01 sec)

mysql> insert into testjson set j='0';
Query OK, 1 row affected (0.00 sec)

mysql> insert into testjson set j='null';
Query OK, 1 row affected (0.00 sec)

mysql> insert into testjson set j='1';
Query OK, 1 row affected (0.00 sec)

mysql> select j from testjson where j;
+------+
| j    |
+------+
| 0    |
| null |
| 1    |
+------+
3 rows in set (0.00 sec)

In MySQL5.7

mysql> create table testjson(j json);
Query OK, 0 rows affected (0.02 sec)

mysql> insert into testjson set j='0';
Query OK, 1 row affected (0.01 sec)

mysql> insert into testjson set j='null';
Query OK, 1 row affected (0.00 sec)

mysql> insert into testjson set j='1';
Query OK, 1 row affected (0.00 sec)

mysql> select j from testjson where j;
+------+
| j    |
+------+
| 1    |
+------+
1 row in set, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+---------------------------------------------------------------+
| Level   | Code | Message                                                       |
+---------+------+---------------------------------------------------------------+
| Warning | 3156 | Invalid JSON value for CAST to INTEGER from column j at row 2 |
+---------+------+---------------------------------------------------------------+
1 row in set (0.00 sec)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I will investigate it.

@@ -1850,6 +1850,12 @@ func (s *testSuiteP1) TestGeneratedColumnRead(c *C) {
result = tk.MustQuery(`SELECT * FROM test_gc_read_m`)
result.Check(testkit.Rows(`1 2 4`, `2 3 6`))

// Test for issue #12233
tk.MustExec(`create table testjson( id int auto_increment not null primary key, j json )default charset=utf8 engine=innodb;`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you simplify this table schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I had simplified the table schema.

executor/executor_test.go Outdated Show resolved Hide resolved
@@ -1850,6 +1850,12 @@ func (s *testSuiteP1) TestGeneratedColumnRead(c *C) {
result = tk.MustQuery(`SELECT * FROM test_gc_read_m`)
result.Check(testkit.Rows(`1 2 4`, `2 3 6`))

// Test for issue #12233
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not put it in TestGeneratedColumnRead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you. I had removed this comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, maybe you don't get me.

Your test case is in TestGeneratedColumnRead, but it should not be here.

@tsthght
Copy link
Contributor Author

tsthght commented Oct 15, 2019

/rebuild

@tsthght
Copy link
Contributor Author

tsthght commented Oct 18, 2019

/rebuild

@zyxbest
Copy link
Contributor

zyxbest commented Oct 18, 2019

/run-check_dev

@qw4990 qw4990 removed their request for review November 26, 2019 07:33
@Deardrops
Copy link
Contributor

Deardrops commented Dec 3, 2019

@tsthght Friendly ping, any update for this PR?

@sre-bot
Copy link
Contributor

sre-bot commented Jan 4, 2020

@tsthght, please update your pull request.

1 similar comment
@sre-bot
Copy link
Contributor

sre-bot commented Jan 11, 2020

@tsthght, please update your pull request.

@sre-bot
Copy link
Contributor

sre-bot commented Jan 27, 2020

@tsthght PR closed due to no update for a long time. Feel free to reopen it anytime.

@sre-bot sre-bot closed this Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression contribution This PR is from a community contributor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants