Skip to content

copr: implement json cast as bool#9286

Merged
ti-srebot merged 9 commits intotikv:masterfrom
wshwsh12:json
Dec 19, 2020
Merged

copr: implement json cast as bool#9286
ti-srebot merged 9 commits intotikv:masterfrom
wshwsh12:json

Conversation

@wshwsh12
Copy link
Contributor

@wshwsh12 wshwsh12 commented Dec 16, 2020

Signed-off-by: wshwsh12 793703860@qq.com

What problem does this PR solve?

Issue Number: close pingcap/tidb#12233

Problem Summary: port pingcap/tidb#18948

What is changed and how it works?

Proposal: xxx

What's Changed:

Related changes

Check List

Tests

  • Unit test
  • Integration test

Side effects

Release note

  • Implement cast json as bool.

Signed-off-by: wshwsh12 <793703860@qq.com>
@wshwsh12
Copy link
Contributor Author

/cc @skyzh

@ti-srebot ti-srebot requested a review from skyzh December 17, 2020 02:29
Copy link
Member

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

Good work!

Datum::Time(t) => Some(!t.is_zero()),
Datum::Dur(d) => Some(!d.is_zero()),
Datum::Dec(d) => Some(ConvertTo::<f64>::convert(&d, ctx)?.round() != 0f64),
Datum::Json(j) => Some(j.as_ref().as_mysql_bool(ctx).unwrap()),
Copy link
Member

Choose a reason for hiding this comment

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

Let's replace unwrap() with ? here, in case that future work makes as_mysql_bool returns Err.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

JsonType::I64 => self.get_i64() == 0,
JsonType::U64 => self.get_u64() == 0,
JsonType::Double => self.get_double() == 0f64,
JsonType::String => false,
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to check the json string's length here?

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 don't think so. I have a test in MySQL8.0.

MySQL [test]> select *,json_extract(j, '$.test') from testjson where json_extract(j, '$.test');
+----+-----------------+---------------------------+
| id | j               | json_extract(j, '$.test') |
+----+-----------------+---------------------------+
|  1 | {"test": ""}    | ""                        |
|  2 | {"test": ""}    | ""                        |
|  3 | {"test": true}  | true                      |
|  4 | {"test": false} | false                     |
+----+-----------------+---------------------------+
4 rows in set, 1 warning (0.000 sec)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the logic here is correctly, here always return false no matter the value is.

Maybe we can add ref: https://dev.mysql.com/doc/refman/8.0/en/json.html#Converting%20between%20JSON%20and%20non-JSON%20values here?

MariaDB [test]> CREATE TABLE JSON (c JSON);
MariaDB [test]> insert into json values row('{"a":"","b":"abc"}');
MariaDB [test]> select if(JSON_EXTRACT(c, '$.a') = true, 'TRUE', 'FALSE') from JSON;
+----------------------------------------------------+
| if(JSON_EXTRACT(c, '$.a') = true, 'TRUE', 'FALSE') |
+----------------------------------------------------+
| FALSE                                              |
+----------------------------------------------------+
1 row in set (0.001 sec)
MariaDB [test]> select if(JSON_EXTRACT(c, '$.b') = true, 'TRUE', 'FALSE') from JSON;
+----------------------------------------------------+
| if(JSON_EXTRACT(c, '$.b') = true, 'TRUE', 'FALSE') |
+----------------------------------------------------+
| FALSE                                              |
+----------------------------------------------------+
1 row in set (0.001 sec)

wshwsh12 and others added 3 commits December 17, 2020 11:50
Signed-off-by: wshwsh12 <793703860@qq.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: wshwsh12 <793703860@qq.com>
Copy link
Member

@Xuanwo Xuanwo 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 17, 2020
@wshwsh12 wshwsh12 requested a review from skyzh December 17, 2020 08:23
@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Dec 17, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 17, 2020
@skyzh skyzh added the status/can-merge Indicates a PR has been approved by a committer. label Dec 17, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@wshwsh12 merge failed.

@wshwsh12
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@wshwsh12 merge failed.

@Xuanwo
Copy link
Member

Xuanwo commented Dec 19, 2020

/merge

@ti-srebot
Copy link
Contributor

@Xuanwo Oops! auto merge is restricted to Committers of the SIG.You are not a committer or co-leader or leader.

@skyzh
Copy link
Member

skyzh commented Dec 19, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 06c9683 into tikv:master Dec 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JSON_EXTRACT fails to cast as bool

5 participants