You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ZENOTME (via GitHub)" <gi...@apache.org> on 2024/12/13 16:15:51 UTC
[PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
ZENOTME opened a new pull request, #795:
URL: https://github.com/apache/iceberg-rust/pull/795
We should deserialize according to order of struct type rather than the deserialize value.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#issuecomment-2635824232
> Hi, I'm a bit confused about why we need to care about this. [De]serialization is a very format-specific task, and it's really challenging to ensure our implementations meet all format requirements. I'm a bit concerned about the additional cost we incur to achieve this. Doesn't it seem fine as long as it works well with Avro?
According to this, it works if we only use the RawLiteralEnum for avro. Maybe we can just add comment for this and rollback this commit?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo closed pull request #795: fix: gurantee the deserialize order of struct is same as the struct type
URL: https://github.com/apache/iceberg-rust/pull/795
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on code in PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#discussion_r1899896614
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -2728,22 +2730,24 @@ mod _serde {
optional: _,
}) => match ty {
Type::Struct(struct_ty) => {
- let iters: Vec<Option<Literal>> = required
- .into_iter()
- .map(|(field_name, value)| {
- let field = struct_ty
- .field_by_name(field_name.as_str())
- .ok_or_else(|| {
- invalid_err_with_reason(
- "record",
- &format!("field {} is not exist", &field_name),
- )
- })?;
- let value = value.try_into(&field.field_type)?;
- Ok(value)
+ let mut value_map: HashMap<String, RawLiteralEnum> =
Review Comment:
Because when deserilaize, None will be convert to RawLiteralEnum::Null. So there is no need for optional here. And actually, when deserialize, we don't know the schema, so that we don't know which value is optional and which is required, we just know which value is valid and which is none.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#discussion_r1899878300
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -2728,22 +2730,24 @@ mod _serde {
optional: _,
}) => match ty {
Type::Struct(struct_ty) => {
- let iters: Vec<Option<Literal>> = required
- .into_iter()
- .map(|(field_name, value)| {
- let field = struct_ty
- .field_by_name(field_name.as_str())
- .ok_or_else(|| {
- invalid_err_with_reason(
- "record",
- &format!("field {} is not exist", &field_name),
- )
- })?;
- let value = value.try_into(&field.field_type)?;
- Ok(value)
+ let mut value_map: HashMap<String, RawLiteralEnum> =
Review Comment:
I don't quite understand why we ignore `optional` here?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#discussion_r1887077718
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -3604,4 +3608,29 @@ mod tests {
assert_eq!(result, expected);
}
+
+ #[test]
+ fn test_record_ser_de() {
+ let expected_literal = Literal::Struct(Struct::from_iter(vec![
+ Some(Literal::Primitive(PrimitiveLiteral::Int(1))),
+ Some(Literal::Primitive(PrimitiveLiteral::String(
+ "bar".to_string(),
+ ))),
+ None,
+ Some(Literal::Primitive(PrimitiveLiteral::Int(1000))),
+ ]));
+ let fields = Type::Struct(StructType::new(vec![
+ NestedField::required(2, "id", Type::Primitive(PrimitiveType::Int)).into(),
+ NestedField::optional(3, "name", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::optional(4, "address", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::required(5, "extra", Type::Primitive(PrimitiveType::Int)).into(),
+ ]));
+
+ let raw_literal = RawLiteral::try_from(expected_literal.clone(), &fields).unwrap();
+ let serialized = serde_json::to_string(&raw_literal).unwrap();
+ let deserialized: RawLiteral = serde_json::from_str(&serialized).unwrap();
+ let deserialized_literal = deserialized.try_into(&fields).unwrap().unwrap();
+
+ assert_eq!(expected_literal, deserialized_literal);
Review Comment:
Hi, I'm a bit confused about why we need to care about this. [De]serialization is a very format-specific task, and it's really challenging to ensure our implementations meet all format requirements. I'm a bit concerned about the additional cost we incur to achieve this. Doesn't it seem fine as long as it works well with Avro?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#discussion_r1887083904
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -3604,4 +3608,29 @@ mod tests {
assert_eq!(result, expected);
}
+
+ #[test]
+ fn test_record_ser_de() {
+ let expected_literal = Literal::Struct(Struct::from_iter(vec![
+ Some(Literal::Primitive(PrimitiveLiteral::Int(1))),
+ Some(Literal::Primitive(PrimitiveLiteral::String(
+ "bar".to_string(),
+ ))),
+ None,
+ Some(Literal::Primitive(PrimitiveLiteral::Int(1000))),
+ ]));
+ let fields = Type::Struct(StructType::new(vec![
+ NestedField::required(2, "id", Type::Primitive(PrimitiveType::Int)).into(),
+ NestedField::optional(3, "name", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::optional(4, "address", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::required(5, "extra", Type::Primitive(PrimitiveType::Int)).into(),
+ ]));
+
+ let raw_literal = RawLiteral::try_from(expected_literal.clone(), &fields).unwrap();
+ let serialized = serde_json::to_string(&raw_literal).unwrap();
+ let deserialized: RawLiteral = serde_json::from_str(&serialized).unwrap();
+ let deserialized_literal = deserialized.try_into(&fields).unwrap().unwrap();
+
+ assert_eq!(expected_literal, deserialized_literal);
Review Comment:
Ah, missed that. Let me unblock this for now 👍
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#issuecomment-2644493012
> > According to this, it works if we only use the RawLiteralEnum for avro. Maybe we can just add comment for this and rollback this commit?
>
> Yes, I believe we only need to add comments on this.
https://github.com/apache/iceberg-rust/blob/a22440eb717cbf57fb366ea04853993ac8435337/crates/iceberg/src/spec/values.rs#L2236 Looks we already have a related comment. We can close this PR.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on code in PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#discussion_r1884853773
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -3439,11 +3443,13 @@ mod tests {
"bar".to_string(),
))),
None,
+ Some(Literal::Primitive(PrimitiveLiteral::Int(1000))),
])),
&Type::Struct(StructType::new(vec![
NestedField::required(2, "id", Type::Primitive(PrimitiveType::Int)).into(),
NestedField::optional(3, "name", Type::Primitive(PrimitiveType::String)).into(),
NestedField::optional(4, "address", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::required(5, "extra", Type::Primitive(PrimitiveType::Int)).into(),
Review Comment:
Yes, but I find that it can also pass originally. I'm trying to find the test case that can't pass originally.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#issuecomment-2644490002
> According to this, it works if we only use the RawLiteralEnum for avro. Maybe we can just add comment for this and rollback this commit?
Yes, I believe we only need to add comments on this.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#discussion_r1884868240
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -3439,11 +3443,13 @@ mod tests {
"bar".to_string(),
))),
None,
+ Some(Literal::Primitive(PrimitiveLiteral::Int(1000))),
])),
&Type::Struct(StructType::new(vec![
NestedField::required(2, "id", Type::Primitive(PrimitiveType::Int)).into(),
NestedField::optional(3, "name", Type::Primitive(PrimitiveType::String)).into(),
NestedField::optional(4, "address", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::required(5, "extra", Type::Primitive(PrimitiveType::Int)).into(),
Review Comment:
> Yes, but I find that it can also pass originally. I'm trying to find the test case that can't pass originally.
Thank you, that will be really meaningful.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#discussion_r1886917197
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -3604,4 +3608,29 @@ mod tests {
assert_eq!(result, expected);
}
+
+ #[test]
+ fn test_record_ser_de() {
+ let expected_literal = Literal::Struct(Struct::from_iter(vec![
+ Some(Literal::Primitive(PrimitiveLiteral::Int(1))),
+ Some(Literal::Primitive(PrimitiveLiteral::String(
+ "bar".to_string(),
+ ))),
+ None,
+ Some(Literal::Primitive(PrimitiveLiteral::Int(1000))),
+ ]));
+ let fields = Type::Struct(StructType::new(vec![
+ NestedField::required(2, "id", Type::Primitive(PrimitiveType::Int)).into(),
+ NestedField::optional(3, "name", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::optional(4, "address", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::required(5, "extra", Type::Primitive(PrimitiveType::Int)).into(),
+ ]));
+
+ let raw_literal = RawLiteral::try_from(expected_literal.clone(), &fields).unwrap();
+ let serialized = serde_json::to_string(&raw_literal).unwrap();
+ let deserialized: RawLiteral = serde_json::from_str(&serialized).unwrap();
+ let deserialized_literal = deserialized.try_into(&fields).unwrap().unwrap();
+
+ assert_eq!(expected_literal, deserialized_literal);
Review Comment:
It looks like the whole serialization is off, this should be [done by ID](https://iceberg.apache.org/spec/?column-projection#json-single-value-serialization) instead of name:
I checked out the branch, and it is currently by name:
```json
{
"id": 1,
"extra": 1000,
"name": "bar",
"address": null
}
```
While it should be:
```json
{
"2": 1,
"3": "bar",
"4": null,
"5": 1000
}
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#issuecomment-2644493194
> Looks we already have a related comment. We can close this PR.
Great. Thank you for researching on this.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on code in PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#discussion_r1887057758
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -3604,4 +3608,29 @@ mod tests {
assert_eq!(result, expected);
}
+
+ #[test]
+ fn test_record_ser_de() {
+ let expected_literal = Literal::Struct(Struct::from_iter(vec![
+ Some(Literal::Primitive(PrimitiveLiteral::Int(1))),
+ Some(Literal::Primitive(PrimitiveLiteral::String(
+ "bar".to_string(),
+ ))),
+ None,
+ Some(Literal::Primitive(PrimitiveLiteral::Int(1000))),
+ ]));
+ let fields = Type::Struct(StructType::new(vec![
+ NestedField::required(2, "id", Type::Primitive(PrimitiveType::Int)).into(),
+ NestedField::optional(3, "name", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::optional(4, "address", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::required(5, "extra", Type::Primitive(PrimitiveType::Int)).into(),
+ ]));
+
+ let raw_literal = RawLiteral::try_from(expected_literal.clone(), &fields).unwrap();
+ let serialized = serde_json::to_string(&raw_literal).unwrap();
+ let deserialized: RawLiteral = serde_json::from_str(&serialized).unwrap();
+ let deserialized_literal = deserialized.try_into(&fields).unwrap().unwrap();
+
+ assert_eq!(expected_literal, deserialized_literal);
Review Comment:
This is not same as JSON single-value serialization, th JSON single-value serialization has the specific implementation https://github.com/apache/iceberg-rust/blob/813c2b5610490886e819be97fb5fbf173ad68a58/crates/iceberg/src/spec/values.rs#L1993.
This test case is just to test the normal Serialize implementation, internally it mainly used in avro format. See https://docs.rs/avro-rs/latest/avro_rs/types/struct.Record.html, that's why here record store name and value.
Here I serialize it into json type is to test the reorder case.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#discussion_r1884848752
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -3439,11 +3443,13 @@ mod tests {
"bar".to_string(),
))),
None,
+ Some(Literal::Primitive(PrimitiveLiteral::Int(1000))),
])),
&Type::Struct(StructType::new(vec![
NestedField::required(2, "id", Type::Primitive(PrimitiveType::Int)).into(),
NestedField::optional(3, "name", Type::Primitive(PrimitiveType::String)).into(),
NestedField::optional(4, "address", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::required(5, "extra", Type::Primitive(PrimitiveType::Int)).into(),
Review Comment:
Hi, would you like to add a new test that cover the mis-order cases?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#discussion_r1887570509
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -3604,4 +3608,29 @@ mod tests {
assert_eq!(result, expected);
}
+
+ #[test]
+ fn test_record_ser_de() {
+ let expected_literal = Literal::Struct(Struct::from_iter(vec![
+ Some(Literal::Primitive(PrimitiveLiteral::Int(1))),
+ Some(Literal::Primitive(PrimitiveLiteral::String(
+ "bar".to_string(),
+ ))),
+ None,
+ Some(Literal::Primitive(PrimitiveLiteral::Int(1000))),
+ ]));
+ let fields = Type::Struct(StructType::new(vec![
+ NestedField::required(2, "id", Type::Primitive(PrimitiveType::Int)).into(),
+ NestedField::optional(3, "name", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::optional(4, "address", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::required(5, "extra", Type::Primitive(PrimitiveType::Int)).into(),
+ ]));
+
+ let raw_literal = RawLiteral::try_from(expected_literal.clone(), &fields).unwrap();
+ let serialized = serde_json::to_string(&raw_literal).unwrap();
+ let deserialized: RawLiteral = serde_json::from_str(&serialized).unwrap();
+ let deserialized_literal = deserialized.try_into(&fields).unwrap().unwrap();
+
+ assert_eq!(expected_literal, deserialized_literal);
Review Comment:
Ah, around the same time! I share your concern, and I would like to check later on if we do the field-ID projection properly, but I didn't want to block the release 👍
For Avro it is very simple, it will always be decoded in the same order as the schema (otherwise it will just break). That said, we can rely on the order for V1, but use field-ID-based projection for V2 tables.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#discussion_r1887090289
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -3604,4 +3608,29 @@ mod tests {
assert_eq!(result, expected);
}
+
+ #[test]
+ fn test_record_ser_de() {
+ let expected_literal = Literal::Struct(Struct::from_iter(vec![
+ Some(Literal::Primitive(PrimitiveLiteral::Int(1))),
+ Some(Literal::Primitive(PrimitiveLiteral::String(
+ "bar".to_string(),
+ ))),
+ None,
+ Some(Literal::Primitive(PrimitiveLiteral::Int(1000))),
+ ]));
+ let fields = Type::Struct(StructType::new(vec![
+ NestedField::required(2, "id", Type::Primitive(PrimitiveType::Int)).into(),
+ NestedField::optional(3, "name", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::optional(4, "address", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::required(5, "extra", Type::Primitive(PrimitiveType::Int)).into(),
+ ]));
+
+ let raw_literal = RawLiteral::try_from(expected_literal.clone(), &fields).unwrap();
+ let serialized = serde_json::to_string(&raw_literal).unwrap();
+ let deserialized: RawLiteral = serde_json::from_str(&serialized).unwrap();
+ let deserialized_literal = deserialized.try_into(&fields).unwrap().unwrap();
+
+ assert_eq!(expected_literal, deserialized_literal);
Review Comment:
Mark unresolve for my newly added comment: https://github.com/apache/iceberg-rust/pull/795#discussion_r1887077718
Sorry @Fokko :smile:
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on code in PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#discussion_r1884873126
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -3439,11 +3443,13 @@ mod tests {
"bar".to_string(),
))),
None,
+ Some(Literal::Primitive(PrimitiveLiteral::Int(1000))),
])),
&Type::Struct(StructType::new(vec![
NestedField::required(2, "id", Type::Primitive(PrimitiveType::Int)).into(),
NestedField::optional(3, "name", Type::Primitive(PrimitiveType::String)).into(),
NestedField::optional(4, "address", Type::Primitive(PrimitiveType::String)).into(),
+ NestedField::required(5, "extra", Type::Primitive(PrimitiveType::Int)).into(),
Review Comment:
I have found the reason why this can pass originally: the avro writer will ensure the record order according to the schema: https://github.com/apache/avro-rs/blob/390a150bfc5999eb852c9c0ef40335612f1407b5/avro/src/encode.rs#L247.
However, if we serialize into other formats, e.g. json, the order can't be guaranteed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: gurantee the deserialize order of struct is same as the struct type [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on PR #795:
URL: https://github.com/apache/iceberg-rust/pull/795#issuecomment-2541792027
cc @liurenjie1024 @Fokko @Xuanwo @sdd
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org