Skip to content

Commit 737e069

Browse files
MichaelScofieldMichaelScofield
committed
resolve PR comments
Signed-off-by: luofucong <[email protected]>
1 parent f204903 commit 737e069

File tree

6 files changed

+96
-74
lines changed

6 files changed

+96
-74
lines changed

‎Cargo.lock‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ etcd-client = { git = "https://github.com/GreptimeTeam/etcd-client", rev = "f62d
148148
fst = "0.4.7"
149149
futures = "0.3"
150150
futures-util = "0.3"
151-
greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "09e1425b6de1cdf1434f217e0546febc0de2e093" }
151+
greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "26b4962b69e07a810b5069064320c167cad80ad3" }
152152
hex = "0.4"
153153
http = "1"
154154
humantime = "2.1"

‎src/api/src/helper.rs‎

Lines changed: 74 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15-
use std::collections::{BTreeMap, HashMap, HashSet};
15+
use std::collections::{BTreeMap, HashSet};
1616
use std::sync::Arc;
1717

1818
use common_decimal::Decimal128;
@@ -949,8 +949,11 @@ fn encode_json_value(value: JsonValue) -> v1::JsonValue {
949949
JsonVariant::Object(x) => {
950950
let entries = x
951951
.into_iter()
952-
.map(|(k, v)| (k, helper(v)))
953-
.collect::<HashMap<_, _>>();
952+
.map(|(key, v)| v1::json_object::Entry {
953+
key,
954+
value: Some(helper(v)),
955+
})
956+
.collect::<Vec<_>>();
954957
Some(json_value::Value::Object(JsonObject { entries }))
955958
}
956959
};
@@ -961,7 +964,7 @@ fn encode_json_value(value: JsonValue) -> v1::JsonValue {
961964

962965
fn decode_json_value(value: &v1::JsonValue) -> JsonValueRef<'_> {
963966
let Some(value) = &value.value else {
964-
return ().into();
967+
return JsonValueRef::null();
965968
};
966969
match value {
967970
json_value::Value::Boolean(x) => (*x).into(),
@@ -978,7 +981,12 @@ fn decode_json_value(value: &v1::JsonValue) -> JsonValueRef<'_> {
978981
json_value::Value::Object(x) => x
979982
.entries
980983
.iter()
981-
.map(|(k, v)| (k.as_str(), decode_json_value(v).into_variant()))
984+
.filter_map(|entry| {
985+
entry
986+
.value
987+
.as_ref()
988+
.map(|v| (entry.key.as_str(), decode_json_value(v).into_variant()))
989+
})
982990
.collect::<BTreeMap<_, _>>()
983991
.into(),
984992
}
@@ -1819,7 +1827,7 @@ mod tests {
18191827

18201828
#[test]
18211829
fn test_encode_decode_json_value() {
1822-
let json: JsonValue = ().into();
1830+
let json = JsonValue::null();
18231831
let proto = encode_json_value(json.clone());
18241832
assert!(proto.value.is_none());
18251833
let value = decode_json_value(&proto);
@@ -1885,32 +1893,31 @@ mod tests {
18851893
let value = decode_json_value(&proto);
18861894
assert_eq!(json.as_ref(), value);
18871895

1888-
let json: JsonValue = [("k1", 1i64), ("k2", 2i64), ("k3", 3i64)].into();
1896+
let json: JsonValue = [("k3", 3i64), ("k2", 2i64), ("k1", 1i64)].into();
18891897
let proto = encode_json_value(json.clone());
18901898
assert_eq!(
18911899
proto.value,
18921900
Some(json_value::Value::Object(JsonObject {
1893-
entries: [
1894-
(
1895-
"k1".to_string(),
1896-
v1::JsonValue {
1901+
entries: vec![
1902+
v1::json_object::Entry {
1903+
key: "k1".to_string(),
1904+
value: Some(v1::JsonValue {
18971905
value: Some(json_value::Value::Int(1))
1898-
}
1899-
),
1900-
(
1901-
"k2".to_string(),
1902-
v1::JsonValue {
1906+
}),
1907+
},
1908+
v1::json_object::Entry {
1909+
key: "k2".to_string(),
1910+
value: Some(v1::JsonValue {
19031911
value: Some(json_value::Value::Int(2))
1904-
}
1905-
),
1906-
(
1907-
"k3".to_string(),
1908-
v1::JsonValue {
1912+
}),
1913+
},
1914+
v1::json_object::Entry {
1915+
key: "k3".to_string(),
1916+
value: Some(v1::JsonValue {
19091917
value: Some(json_value::Value::Int(3))
1910-
}
1911-
)
1918+
}),
1919+
},
19121920
]
1913-
.into(),
19141921
}))
19151922
);
19161923
let value = decode_json_value(&proto);
@@ -1920,9 +1927,7 @@ mod tests {
19201927
let proto = encode_json_value(json.clone());
19211928
assert_eq!(
19221929
proto.value,
1923-
Some(json_value::Value::Object(JsonObject {
1924-
entries: HashMap::new(),
1925-
}))
1930+
Some(json_value::Value::Object(JsonObject { entries: vec![] }))
19261931
);
19271932
let value = decode_json_value(&proto);
19281933
assert_eq!(json.as_ref(), value);
@@ -1946,17 +1951,16 @@ mod tests {
19461951
assert_eq!(
19471952
proto.value,
19481953
Some(json_value::Value::Object(JsonObject {
1949-
entries: [
1950-
("null".to_string(), v1::JsonValue { value: None }),
1951-
(
1952-
"bool".to_string(),
1953-
v1::JsonValue {
1954+
entries: vec![
1955+
v1::json_object::Entry {
1956+
key: "bool".to_string(),
1957+
value: Some(v1::JsonValue {
19541958
value: Some(json_value::Value::Boolean(false))
1955-
}
1956-
),
1957-
(
1958-
"list".to_string(),
1959-
v1::JsonValue {
1959+
}),
1960+
},
1961+
v1::json_object::Entry {
1962+
key: "list".to_string(),
1963+
value: Some(v1::JsonValue {
19601964
value: Some(json_value::Value::Array(JsonList {
19611965
items: vec![
19621966
v1::JsonValue {
@@ -1967,48 +1971,49 @@ mod tests {
19671971
},
19681972
]
19691973
}))
1970-
}
1971-
),
1972-
(
1973-
"object".to_string(),
1974-
v1::JsonValue {
1974+
}),
1975+
},
1976+
v1::json_object::Entry {
1977+
key: "null".to_string(),
1978+
value: Some(v1::JsonValue { value: None }),
1979+
},
1980+
v1::json_object::Entry {
1981+
key: "object".to_string(),
1982+
value: Some(v1::JsonValue {
19751983
value: Some(json_value::Value::Object(JsonObject {
1976-
entries: [
1977-
(
1978-
"positive_i".to_string(),
1979-
v1::JsonValue {
1980-
value: Some(json_value::Value::Uint(42))
1981-
}
1982-
),
1983-
(
1984-
"negative_i".to_string(),
1985-
v1::JsonValue {
1984+
entries: vec![
1985+
v1::json_object::Entry {
1986+
key: "negative_i".to_string(),
1987+
value: Some(v1::JsonValue {
19861988
value: Some(json_value::Value::Int(-42))
1987-
}
1988-
),
1989-
(
1990-
"nested".to_string(),
1991-
v1::JsonValue {
1989+
}),
1990+
},
1991+
v1::json_object::Entry {
1992+
key: "nested".to_string(),
1993+
value: Some(v1::JsonValue {
19921994
value: Some(json_value::Value::Object(JsonObject {
1993-
entries: [(
1994-
"what".to_string(),
1995-
v1::JsonValue {
1995+
entries: vec![v1::json_object::Entry {
1996+
key: "what".to_string(),
1997+
value: Some(v1::JsonValue {
19961998
value: Some(json_value::Value::Str(
19971999
"blah".to_string()
19982000
))
1999-
}
2000-
)]
2001-
.into()
2001+
}),
2002+
},]
20022003
}))
2003-
}
2004-
)
2004+
}),
2005+
},
2006+
v1::json_object::Entry {
2007+
key: "positive_i".to_string(),
2008+
value: Some(v1::JsonValue {
2009+
value: Some(json_value::Value::Uint(42))
2010+
}),
2011+
},
20052012
]
2006-
.into(),
20072013
}))
2008-
}
2009-
),
2014+
}),
2015+
},
20102016
]
2011-
.into(),
20122017
}))
20132018
);
20142019
let value = decode_json_value(&proto);

‎src/datatypes/src/json.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -266,7 +266,7 @@ fn encode_json_array_with_context<'a>(
266266
ensure!(
267267
item_type == *current_type,
268268
error::InvalidJsonSnafu {
269-
value: "item types in json array are not all equal"
269+
value: "all items in json array must have the same type"
270270
}
271271
);
272272
} else {

‎src/datatypes/src/json/value.rs‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ use crate::value::{ListValue, ListValueRef, StructValue, StructValueRef, Value,
2929

3030
/// Number in json, can be a positive integer, a negative integer, or a floating number.
3131
/// Each of which is represented as `u64`, `i64` and `f64`.
32+
///
33+
/// This follows how `serde_json` designs number.
3234
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)]
3335
pub enum JsonNumber {
3436
PosInt(u64),
@@ -91,6 +93,11 @@ impl Display for JsonNumber {
9193
}
9294

9395
/// Variants of json.
96+
///
97+
/// This follows how [serde_json::Value] designs except that we only choose to use [BTreeMap] to
98+
/// preserve the fields order by their names in the json object. (By default `serde_json` uses
99+
/// [BTreeMap], too. But it additionally supports "IndexMap" which preserves the order by insertion
100+
/// times of fields.)
94101
#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]
95102
pub enum JsonVariant {
96103
Null,
@@ -138,7 +145,7 @@ impl JsonVariant {
138145

139146
fn as_ref(&self) -> JsonVariantRef<'_> {
140147
match self {
141-
JsonVariant::Null => ().into(),
148+
JsonVariant::Null => JsonVariantRef::Null,
142149
JsonVariant::Bool(x) => (*x).into(),
143150
JsonVariant::Number(x) => match x {
144151
JsonNumber::PosInt(i) => (*i).into(),
@@ -241,6 +248,10 @@ pub struct JsonValue {
241248
}
242249

243250
impl JsonValue {
251+
pub fn null() -> Self {
252+
().into()
253+
}
254+
244255
pub(crate) fn new(json_variant: JsonVariant) -> Self {
245256
Self {
246257
json_type: OnceLock::new(),
@@ -551,6 +562,10 @@ pub struct JsonValueRef<'a> {
551562
}
552563

553564
impl<'a> JsonValueRef<'a> {
565+
pub fn null() -> Self {
566+
().into()
567+
}
568+
554569
pub(crate) fn data_type(&self) -> ConcreteDataType {
555570
ConcreteDataType::Json(self.json_type().clone())
556571
}

‎src/frontend/src/limiter.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,9 @@ impl Limiter {
245245
json_value::Value::Object(object) => object
246246
.entries
247247
.iter()
248-
.map(|(key, val)| key.len() + calc(val))
248+
.flat_map(|entry| {
249+
entry.value.as_ref().map(|v| entry.key.len() + calc(v))
250+
})
249251
.sum(),
250252
}
251253
}

0 commit comments

Comments
 (0)