Copilot commented on code in PR #53:
URL: https://github.com/apache/fluss-rust/pull/53#discussion_r2554605845
##########
crates/fluss/src/metadata/json_serde.rs:
##########
@@ -110,22 +108,46 @@ impl JsonSerde for DataType {
DataType::Binary(_type) => {
obj.insert(Self::FIELD_NAME_LENGTH.to_string(),
json!(_type.length()));
}
+ DataType::Char(_type) => {
+ obj.insert(Self::FIELD_NAME_LENGTH.to_string(),
json!(_type.length()));
+ }
Review Comment:
Duplicate match arm for DataType::Char. The match arm at lines 111-113 is
unreachable and should be removed. The Binary case at lines 108-110 is
correctly positioned after the first Char case, making this duplicate dead code.
```suggestion
```
##########
crates/fluss/src/metadata/json_serde.rs:
##########
@@ -175,6 +264,51 @@ impl JsonSerde for DataType {
}
}
+impl DataField {
+const NAME: &'static str = "name";
+const FIELD_TYPE: &'static str = "field_type";
+const DESCRIPTION: &'static str = "description";
+}
+
+impl JsonSerde for DataField {
+ fn serialize_json(&self) -> Result<Value> {
+ let mut obj = serde_json::Map::new();
+
+ obj.insert(Self::NAME.to_string(), json!(self.name()));
+ obj.insert(
+ Self::FIELD_TYPE.to_string(),
+ self.data_type.serialize_json()?,
+ );
+
+ if let Some(description) = &self.description {
+ obj.insert(Self::DESCRIPTION.to_string(), json!(description));
+ }
+
+ Ok(Value::Object(obj))
+ }
+
+ fn deserialize_json(node: &Value) -> Result<DataField> {
+ let name = node
+ .get(Self::NAME)
+ .and_then(|v| v.as_str())
+ .ok_or_else(|| JsonSerdeError(format!("Missing required field: {}",
Self::NAME)))?
+ .to_string();
+
+ let field_type_node = node
+ .get(Self::FIELD_TYPE)
Review Comment:
[nitpick] Inconsistent indentation: the continuation of the method chain
should be indented properly. Line 298 should align with the opening of the
chain on line 297.
```suggestion
.get(Self::FIELD_TYPE)
```
##########
crates/fluss/src/metadata/json_serde.rs:
##########
@@ -175,6 +264,51 @@ impl JsonSerde for DataType {
}
}
+impl DataField {
+const NAME: &'static str = "name";
+const FIELD_TYPE: &'static str = "field_type";
+const DESCRIPTION: &'static str = "description";
+}
+
+impl JsonSerde for DataField {
+ fn serialize_json(&self) -> Result<Value> {
+ let mut obj = serde_json::Map::new();
+
+ obj.insert(Self::NAME.to_string(), json!(self.name()));
+ obj.insert(
+ Self::FIELD_TYPE.to_string(),
+ self.data_type.serialize_json()?,
+ );
+
+ if let Some(description) = &self.description {
+ obj.insert(Self::DESCRIPTION.to_string(), json!(description));
+ }
+
+ Ok(Value::Object(obj))
+ }
+
+ fn deserialize_json(node: &Value) -> Result<DataField> {
+ let name = node
+ .get(Self::NAME)
+ .and_then(|v| v.as_str())
+ .ok_or_else(|| JsonSerdeError(format!("Missing required field: {}",
Self::NAME)))?
+ .to_string();
Review Comment:
[nitpick] Inconsistent indentation: the continuation of the method chain
should be indented properly. Line 292 should align with the opening of the
chain on line 291.
```suggestion
.get(Self::NAME)
.and_then(|v| v.as_str())
.ok_or_else(|| JsonSerdeError(format!("Missing required field:
{}", Self::NAME)))?
.to_string();
```
##########
crates/fluss/src/metadata/json_serde.rs:
##########
@@ -307,16 +441,16 @@ impl TableDescriptor {
fn deserialize_properties(node: &Value) -> Result<HashMap<String, String>>
{
let obj = node
- .as_object()
- .ok_or_else(|| JsonSerdeError("Properties should be an
object".to_string()))?;
+ .as_object()
Review Comment:
[nitpick] Inconsistent indentation: the continuation of the method chain
should be indented properly. Line 444 should align with the opening of the
chain on line 443.
```suggestion
.as_object()
```
##########
crates/fluss/src/metadata/json_serde.rs:
##########
@@ -175,6 +264,51 @@ impl JsonSerde for DataType {
}
}
+impl DataField {
+const NAME: &'static str = "name";
+const FIELD_TYPE: &'static str = "field_type";
+const DESCRIPTION: &'static str = "description";
Review Comment:
Constants should be declared as associated constants (e.g., `const NAME:
&'static str = \"name\";` should be `pub const NAME: &'static str = \"name\";`)
or moved inside the impl block for DataField to match the pattern used in other
types like DataType, Column, and Schema in this file.
```suggestion
pub const NAME: &'static str = "name";
pub const FIELD_TYPE: &'static str = "field_type";
pub const DESCRIPTION: &'static str = "description";
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
