Skip to content

Support 'aliases' in fields#84

Closed
jiak94 wants to merge 7 commits intoflavray:masterfrom
jiak94:resolve_schema
Closed

Support 'aliases' in fields#84
jiak94 wants to merge 7 commits intoflavray:masterfrom
jiak94:resolve_schema

Conversation

@jiak94
Copy link
Contributor

@jiak94 jiak94 commented Jun 6, 2019

when reader_schema specified a aliases in any record in field of a record, should match them before looking for default if name could not find any thing

jiak94 and others added 6 commits March 9, 2019 14:33
fix enum type deserialization bug
Signed-off-by: Jiakuan Li <jiakuan.li@yitu-inc.com>
fixes broken cargo test and add a new feature that handle the Unknown Record (already define)

Signed-off-by: Jiakuan Li <jiakuan.li@yahoo.com>
@jiak94
Copy link
Contributor Author

jiak94 commented Jun 30, 2019

{  
   "name":"person",
   "type":"record",
   "fields":[  
      {  
         "name":"firstname",
         "type":"string"
      },
      {  
         "name":"lastname",
         "type":"string"
      },
      {  
         "name":"address",
         "type":{  
            "type":"record",
            "name":"AddressUSRecord",
            "fields":[  
               {  
                  "name":"streetaddress",
                  "type":"string"
               },
               {  
                  "name":"city",
                  "type":"string"
               }
            ]
         }
      },
      {  
         "name":"test",
         "type":"AddressUSRecord"
      }
   ]
}

for the schema like above, avro-rs will throw an Error as unknown type of AddressUSRecord.

pub struct RecordField {
/// Name of the field.
pub name: String,
// pub name: String,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your editor is messing up the linting, which is probably the reason why the tests are failing. Perhaps you could try running cargo fmt?

@poros
Copy link
Collaborator

poros commented Nov 16, 2019

I think this is going to clash quite badly with #99 , being that one such a big rewrite.
It's kinda hard to tell whether we should go ahead with both at the same time or not...

aliases,
};

// TODO: "type" = "<record name>"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this TODO still valid?

for field in fields {
// This clone is also expensive. See if we can do away with it...
items.push((field.name.clone(), decode(&field.schema, reader)?));
items.push((field.name.name.clone(), decode(&field.schema, reader)?));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do understand where you are coming from, but name.name sounds pretty weird, honestly...

impl RecordField {
/// Parse a `serde_json::Value` into a `RecordField`.
fn parse(field: &Map<String, Value>, position: usize) -> Result<Self, Error> {
fn parse(field: &Map<String, Value>, position: usize, parsed_record: &mut HashMap<String, Schema>) -> Result<Self, Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I like this additional argument that we have to carry around everywhere. It is pretty obscure and it's adding quite a bit of complexity (and clone)...

@poros
Copy link
Collaborator

poros commented Jan 25, 2021

Hello! As of today, I am stepping down as a maintainer of this library for a while (see #174). Please ping @flavray from now on.
Thanks again for your contribution :)

@flavray
Copy link
Owner

flavray commented Jan 28, 2021

This PR has been open for more than a year and it looks like no progress has been made for a while. Our codebase has quite diverged since this was open, too. 😄
Closing this, feel free to re-open if more progress is made!

@flavray flavray closed this Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants