JosephLenton commented on code in PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#discussion_r4538445507


##########
crates/iceberg/src/spec/values/serde.rs:
##########
@@ -106,6 +109,7 @@ pub(crate) mod _serde {
         Double(f64),
         String(String),
         Bytes(ByteBuf),
+        Fixed(#[serde(serialize_with = 
"apache_avro::serde::fixed::serialize")] ByteBuf),

Review Comment:
   Hey @blackmwk , can I please double check the changes you want me to make 
before I go ahead and do them. Just to make sure I don't implement something 
different.
   
   I am reading your comment as _'do not call apache_avro::serde::to_value, and 
replace it with our own converter.'_ Is that right?
   
   Be aware this will have these changes:
    1. `Datum` is public. Calling `to_value( Datum::uuid() )` will continue to 
give the wrong result. That is a bug.
    2. It will mean we will have two ways of turning `RawLiteralEnum` into an 
Avro Value, and the official Avro approach is broken.
    4. Adding `fixed::serialize` only affects `apache_avro::serde::to_value`. 
The JSON serialiser is unaffected by that one line.
    5. Adding a custom `to_avro_value() will add 100+ lines to this PR.
   
   ^ Are you happy with the above changes, or am I misunderstanding 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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to