refactor: Removing type infromation in Struct literal.
#103
Conversation
crates/iceberg/src/spec/values.rs
Outdated
| @@ -554,13 +554,14 @@ impl From<&Literal> for JsonValue { | |||
| PrimitiveLiteral::Decimal(_) => todo!(), | |||
| }, | |||
| Literal::Struct(s) => { | |||
There was a problem hiding this comment.
Hey @liurenjie1024 how should we design this method ? Since the iterator only return the value instead of the (id, value, name) tuple. I'm having trouble setting up the test for this method.
There was a problem hiding this comment.
Update: I used enumerate to account for the id property in the json object. I don't know if this is the correct procedure.
There was a problem hiding this comment.
Sorry for late reply. Is this method really useful? I think we should expose this method
There was a problem hiding this comment.
Isn't the this Struct method supposed to return a hashmap ? The StructType for iceberg requires a NestedField with the id and value.
There was a problem hiding this comment.
I think it's supposed to return an json object, and the field names should match struct field names. But this is not feasible without schema. This method is the correct approach I think.
There was a problem hiding this comment.
Sorry, I mean we should remove the total implementation of From<&Literal> for JsonValue. Since we have decided to remove type info for Struct, this method signature is incorrect. The conversion from/to json should delegate to ser/de module.
There was a problem hiding this comment.
serde_json doesn't seem to have a method to extract the JsonValue from the Literal. This was only possible through From<&Literal> for JsonValue
This line raises a compiler error when I removed From<&Literal> for JsonValue
fn check_json_serde(json: &str, expected_literal: Literal, expected_type: &Type) {
...
let expected_json_value: JsonValue = (&expected_literal).into(); // error raised hereError:
^^^^ the trait `From<&values::Literal>` is not implemented for `serde_json::Value`There was a problem hiding this comment.
Yes, after removing this, directly ser/de from json no longer works. The correct way to do this is to ser/de is using the ser/de module, you can see this method as example. It delegates ser/de to serializaiton/deserialization system.
There was a problem hiding this comment.
So you are saying I should refactor check_json_serde to follow the structure of check_convert_with_avro ?
Also just a reminder, but the code doesn't build when I remove From<&values::Literal> so I don't think removing it entirely will work @liurenjie1024
There was a problem hiding this comment.
Hi, @mobley-trent Yes, we are planning to delegate conversion between literal values and json values to ser/de module, just like what we did with avro. You're right complete removing From<&values::Literal> will not work before that. I think we can hold on this pr before that the json conversion was finished.
|
cc @mobley-trent Could we close this? The issue has been resolve by #120 |
Closes #96