mysql/json: support comparison #1893

AndreMouche · 2017-06-06T03:34:32Z

Hi,
This PR init comparison for type json. It's based on (#1874) ,we could start to review once PR-1874 has been merged:

mysql:add type json #1874

Meanwhile, the related PR in tidb is (pingcap/tidb#3307) and related source file is compare.go

@BusyJay @hicqu @andelf PTAL

hicqu · 2017-06-06T03:55:09Z

src/util/codec/mysql/json.rs

+                TYPE_CODE_DOUBLE | TYPE_CODE_I64 => {
+                    let left_data = self.as_f64().unwrap();
+                    let right_data = right.as_f64().unwrap();
+                    left_data.partial_cmp(&right_data)


here do we need consider precision?

andelf · 2017-06-13T12:12:02Z

src/util/codec/mysql/json.rs

@@ -12,6 +12,7 @@
 // limitations under the License.

 use super::Result;
+use std::cmp::{Ordering, Ord, PartialEq, PartialOrd};


~~Ord, PartialEq, PartialOrd~~

andelf · 2017-06-13T12:12:24Z

src/util/codec/mysql/json.rs

@@ -31,19 +32,42 @@ const TYPE_CODE_I64: u8 = 0x09;
 const TYPE_CODE_DOUBLE: u8 = 0x0b;
 const TYPE_CODE_STRING: u8 = 0x0c;

+#[allow(dead_code)]


#![allow(dead_code)]

andelf · 2017-06-13T12:12:51Z

src/util/codec/mysql/json.rs

+#[allow(dead_code)]
+const PRECEDENCE_DATE: i32 = -6;
+const PRECEDENCE_BOOLEAN: i32 = -7;
+const PRECEDENCE_ARRAY: i32 = -8;


why negative

They are copied from tidb @andelf

andelf · 2017-06-13T12:13:51Z

src/util/codec/mysql/json.rs


-// Json implement type json used in tikv, it specifies the following
+// Json implements type json used in tikv, it specifies the following


andelf · 2017-06-13T12:20:37Z

src/util/codec/mysql/json.rs

+            return match self.get_type_code() {
+                TYPE_CODE_LITERAL => {
+                    let left_data = self.as_literal().unwrap();
+                    let right_data = right.as_literal().unwrap();


will fail, use Option::None

ref doc:

impl<T: PartialOrd> PartialOrd for Option<T> impl<T: PartialOrd, E: PartialOrd> PartialOrd for Result<T, E>

This will give you some hint for how to implement comparison neatly. 😆

address comment @andelf

andelf · 2017-06-13T12:24:07Z

src/util/codec/mysql/json.rs

+
+        let left_data = self.as_f64();
+        let right_data = right.as_f64();
+        // tidb treat boolean as integer, but boolean is different from integer in JSON.


andelf · 2017-06-13T12:26:27Z

src/util/codec/mysql/json.rs

+    }
+}
+
+impl Eq for Json {}


Eq, Ord is needless and theoretically wrong?

andelf · 2017-06-13T12:28:04Z

src/util/codec/mysql/json.rs

+            (&json_bool_false, &json_bool_true),
+        ];
+
+        for (left, right) in test_cases {


for (&left, &right) in

It would be failed with

722 | for (&left, &right) in test_cases { | ^----- | || | |hint: to prevent move, use `ref right` or `ref mut right` | cannot move out of borrowed content

@andelf

andelf · 2017-06-14T07:40:46Z

src/util/codec/mysql/json.rs

@@ -484,7 +613,7 @@ pub trait JsonDecoder: NumberDecoder {
    }

    fn decode_json_item(&mut self, values_data: &[u8], data_start_position: u32) -> Result<Json> {
-        let mut entry = vec![0;LENGTH_VALUE_ENTRY];
+        let mut entry = vec![0;VALUE_ENTRY_LEN];


andelf · 2017-06-14T07:42:01Z

LGTM

AndreMouche · 2017-06-14T08:35:18Z

@hicqu @hhkbp2 @lishihai9017 PTAL,THX

hhkbp2 · 2017-06-15T03:16:29Z

src/util/codec/mysql/json.rs

+        let json_array_big = Json::from_str(r#"["a", "c"]"#).unwrap();
+        let json_array_small = Json::from_str(r#"["a", "b"]"#).unwrap();
+        let json_object = Json::from_str(r#"{"a": "b"}"#).unwrap();
+        let test_cases = vec![


IMO it's better to group the test case in orders like what's in tidb:

values of same type

values of different types

hhkbp2 · 2017-06-15T03:22:19Z

src/util/codec/mysql/json.rs

+const PRECEDENCE_ARRAY: i32 = -8;
+const PRECEDENCE_OBJECT: i32 = -9;
+const PRECEDENCE_STRING: i32 = -10;
+const PRECEDENCE_NUMBER: i32 = -11;


IMO it's better to define

PRECEDENCE_INTEGER = -11 PRECEDENCE_DOUBLE = -11

to map the real types, it's easier to understand since they are listed in real types in MySQL docs and in tidb's code.

This would caused the following warning:

this `match` has identical arm bodies --> src/util/codec/mysql/json.rs:118:32 | 118 | Json::Double(_) => PRECEDENCE_DOUBLE, | ^^^^^^^^^^^^^^^^^ | = note: #[warn(match_same_arms)] on by default note: same as this --> src/util/codec/mysql/json.rs:117:29 | 117 | Json::I64(_) => PRECEDENCE_INTEGER, | ^^^^^^^^^^^^^^^^^ note: consider refactoring into `Json::I64(_) | Json::Double(_)`

@hhkbp2

hhkbp2 · 2017-06-15T03:23:04Z

Rest LGTM

hhkbp2 · 2017-06-15T03:44:21Z

src/util/codec/mysql/json.rs

+        let precedence_diff = self.get_precedence() - right.get_precedence();
+        if precedence_diff == 0 {
+            return match self.get_type_code() {
+                TYPE_CODE_LITERAL => {


It could take the shortcut to skip running the comparison between Json::Nones.

address comment @hhkbp2

hhkbp2 · 2017-06-15T08:31:28Z

LGTM

hicqu · 2017-06-15T11:36:05Z

src/util/codec/mysql/json.rs

+            ("1.5", "2"),
+            ("1.5", "false"),
+            ("true", "1.5"),
+            ("true", "2"),


it's not compatible with MySQL 5.7, although we don't have better implement. But we should comment it here.

hicqu · 2017-06-15T11:37:03Z

LGTM.

AndreMouche added 27 commits May 26, 2017 14:38

mysql:init type json

eccec88

mysql:init test for json

df76d53

mysql:adjust code

939152e

mysql:adjust code

4d01203

Merge branch 'master' into shirly/json_binary

d786e25

mysql:add type i64 for json

3208377

mysql:merge master

2ea910d

mysql:address comments

1e728b3

mysql:adjust cargo.toml

a500aa7

mysql:merge master && fix conflicts

e4bf28e

Merge branch 'master' into shirly/json_binary

6b62f9e

mysql:address comments

6e432c4

Merge branch 'master' into shirly/json_binary

e9554a9

executor:address comments

a99bf52

mysql:address comments

1d854f7

Merge branch 'master' into shirly/json_binary

055eee7

mysql:adjust test according tidb

dd6af79

mysql:merge master

20a29ab

mysql:address comment

88d755b

Merge branch 'master' into shirly/json_binary

7055874

mysql:merge master && fix conflicts

52169f0

json:support cmp

669a004

json:address comment

171d6a8

json:merge json_binary

8a31e02

json:address comments

ba1cc7b

Merge branch 'master' into shirly/json_binary

cb0fbb7

json:merge json_binary

4a578dc

hicqu reviewed Jun 6, 2017

View reviewed changes

json:address comment

78d3da7

AndreMouche changed the title ~~[Pending] mysql/json: support comparison~~ [WIP] mysql/json: support comparison Jun 6, 2017

json:address comment in json_binary

271e1e9

andelf reviewed Jun 13, 2017

View reviewed changes

src/util/codec/mysql/json.rs Outdated

}

}

impl Eq for Json {}

Copy link

Contributor

andelf Jun 13, 2017

Choose a reason for hiding this comment

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

Eq, Ord is needless and theoretically wrong?

andelf reviewed Jun 13, 2017

View reviewed changes

AndreMouche added 2 commits June 13, 2017 21:35

json:address comments

c17d7a7

json_cmp:address comments

dd4878e

andelf reviewed Jun 14, 2017

View reviewed changes

AndreMouche added 2 commits June 14, 2017 16:31

json_cmp:address comments

bda1d87

json_cmp:address comments

920185e

Merge branch 'master' into shirly/json_cmp

9cac71a

hhkbp2 reviewed Jun 15, 2017

View reviewed changes

AndreMouche added 3 commits June 15, 2017 15:18

mysq/json:address comments

757d390

mysql/json:merge master

00dbb79

mysql/json:address comments

4683be4

hhkbp2 approved these changes Jun 15, 2017

View reviewed changes

hhkbp2 merged commit f463c32 into master Jun 15, 2017

hhkbp2 deleted the shirly/json_cmp branch June 15, 2017 08:42

hicqu reviewed Jun 15, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mysql/json: support comparison #1893

mysql/json: support comparison #1893

AndreMouche commented Jun 6, 2017 •

edited by hhkbp2

hicqu Jun 6, 2017

andelf Jun 13, 2017

andelf Jun 13, 2017 •

edited

andelf Jun 13, 2017

AndreMouche Jun 13, 2017

andelf Jun 13, 2017

andelf Jun 13, 2017

andelf Jun 13, 2017

AndreMouche Jun 14, 2017 •

edited

andelf Jun 13, 2017

andelf Jun 13, 2017

andelf Jun 13, 2017

AndreMouche Jun 14, 2017 •

edited

andelf Jun 14, 2017

andelf commented Jun 14, 2017

AndreMouche commented Jun 14, 2017

hhkbp2 Jun 15, 2017

hhkbp2 Jun 15, 2017

AndreMouche Jun 15, 2017

hhkbp2 commented Jun 15, 2017

hhkbp2 Jun 15, 2017 •

edited

AndreMouche Jun 15, 2017

hhkbp2 commented Jun 15, 2017

hicqu Jun 15, 2017

hicqu commented Jun 15, 2017


		// Json implement type json used in tikv, it specifies the following
		// Json implements type json used in tikv, it specifies the following

mysql/json: support comparison #1893

mysql/json: support comparison #1893

Conversation

AndreMouche commented Jun 6, 2017 • edited by hhkbp2

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andelf Jun 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreMouche Jun 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreMouche Jun 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andelf commented Jun 14, 2017

AndreMouche commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhkbp2 commented Jun 15, 2017

hhkbp2 Jun 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhkbp2 commented Jun 15, 2017

Choose a reason for hiding this comment

hicqu commented Jun 15, 2017

AndreMouche commented Jun 6, 2017 •

edited by hhkbp2

andelf Jun 13, 2017 •

edited

AndreMouche Jun 14, 2017 •

edited

AndreMouche Jun 14, 2017 •

edited

hhkbp2 Jun 15, 2017 •

edited