fix: gurantee the deserialize order of struct is same as the struct type

[ZENOTME]

We should deserialize according to order of struct type rather than the deserialize value.

[ZENOTME]

cc @liurenjie1024 @Fokko @Xuanwo @sdd

[Xuanwo]

Hi, would you like to add a new test that cover the mis-order cases?

[ZENOTME]

Yes, but I find that it can also pass originally. I'm trying to find the test case that can't pass originally.

[Xuanwo]

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.

[ZENOTME]

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.

[Fokko]

It looks like the whole serialization is off, this should be done by ID instead of name:

I checked out the branch, and it is currently by name:

{
    "id": 1,
    "extra": 1000,
    "name": "bar",
    "address": null
}

While it should be:

{
    "2": 1,
    "3": "bar",
    "4": null,
    "5": 1000
}

[ZENOTME]

This is not same as JSON single-value serialization, th JSON single-value serialization has the specific implementation

iceberg-rust/crates/iceberg/src/spec/values.rs

Line 1993 in 813c2b5

(Literal::Struct(s), Type::Struct(struct_type)) => {

.
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.

[Xuanwo]

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?

[Fokko]

Ah, missed that. Let me unblock this for now 👍

[Xuanwo]

Mark unresolve for my newly added comment: #795 (comment)

Sorry @Fokko 😄

[Fokko]

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.