jklamer commented on a change in pull request #1602:
URL: https://github.com/apache/avro/pull/1602#discussion_r829139256



##########
File path: lang/rust/avro/src/schema.rs
##########
@@ -259,56 +263,62 @@ impl Name {
     }
 
     /// Parse a `serde_json::Value` into a `Name`.
-    fn parse(complex: &Map<String, Value>) -> AvroResult<Self> {
+    pub fn parse(complex: &Map<String, Value>) -> AvroResult<Self> {

Review comment:
       Ahh this is an artifact of me trying to solve the issue I made the jira 
issue for. Will revert 

##########
File path: lang/rust/avro/src/decode.rs
##########
@@ -68,10 +68,21 @@ fn decode_seq_len<R: Read>(reader: &mut R) -> 
AvroResult<usize> {
 
 /// Decode a `Value` from avro format given its `Schema`.
 pub fn decode<R: Read>(schema: &Schema, reader: &mut R) -> AvroResult<Value> {
-    fn decode0<R: Read>(
+    let rs = ResolvedSchema::try_from(schema)?;
+    decode_internal(schema, rs.get_names(), &None, reader)
+}
+
+fn decode_internal<R: Read>(

Review comment:
       you're right there probably is not? as far as I can tell. 

##########
File path: lang/rust/avro/src/schema.rs
##########
@@ -317,17 +327,86 @@ impl From<&str> for Name {
     }
 }
 
-impl Hash for Name {
-    fn hash<H: Hasher>(&self, state: &mut H) {
-        self.fullname(None).hash(state);
+impl fmt::Display for Name {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str(&self.fullname(None)[..])
+    }
+}
+
+pub(crate) struct ResolvedSchema<'s> {
+    names_ref: HashMap<Name, &'s Schema>,
+    root_schema: &'s Schema,
+}
+
+impl<'s> TryFrom<&'s Schema> for ResolvedSchema<'s> {
+    type Error = Error;
+
+    fn try_from(schema: &'s Schema) -> AvroResult<Self> {
+        let names = HashMap::new();
+        let mut rs = ResolvedSchema {
+            names_ref: names,
+            root_schema: schema,
+        };
+        Self::from_internal(rs.root_schema, &mut rs.names_ref, &None)?;
+        Ok(rs)
     }
 }
 
-impl Eq for Name {}
+impl<'s> ResolvedSchema<'s> {
+    pub fn get_names(&self) -> &HashMap<Name, &'s Schema> {
+        &self.names_ref
+    }
 
-impl PartialEq for Name {
-    fn eq(&self, other: &Name) -> bool {
-        self.fullname(None).eq(&other.fullname(None))
+    fn from_internal(
+        schema: &'s Schema,
+        names_ref: &mut HashMap<Name, &'s Schema>,
+        enclosing_namespace: &Namespace,
+    ) -> AvroResult<()> {
+        match schema {
+            Schema::Array(schema) | Schema::Map(schema) => {
+                Self::from_internal(schema, names_ref, enclosing_namespace)
+            }
+            Schema::Union(UnionSchema { schemas, .. }) => {
+                for schema in schemas {
+                    Self::from_internal(schema, names_ref, 
enclosing_namespace)?
+                }
+                Ok(())
+            }
+            Schema::Enum { name, .. } | Schema::Fixed { name, .. } => {
+                let fully_qualified_name = 
name.fully_qualified_name(enclosing_namespace);
+                if names_ref
+                    .insert(fully_qualified_name.clone(), schema)
+                    .is_some()
+                {
+                    Err(Error::AmbiguousSchemaDefinition(fully_qualified_name))
+                } else {
+                    Ok(())
+                }
+            }
+            Schema::Record { name, fields, .. } => {
+                let fully_qualified_name = 
name.fully_qualified_name(enclosing_namespace);
+                if names_ref
+                    .insert(fully_qualified_name.clone(), schema)
+                    .is_some()
+                {
+                    Err(Error::AmbiguousSchemaDefinition(fully_qualified_name))
+                } else {
+                    let record_namespace = 
name.fully_qualified_name(enclosing_namespace).namespace;

Review comment:
       ahhh yes. missed that

##########
File path: lang/rust/avro/src/encode.rs
##########
@@ -45,155 +50,177 @@ fn encode_int(i: i32, buffer: &mut Vec<u8>) {
     zig_i32(i, buffer)
 }
 
-/// Encode a `Value` into avro format.
-///
-/// **NOTE** This will not perform schema validation. The value is assumed to
-/// be valid with regards to the schema. Schema are needed only to guide the
-/// encoding for complex type values.
-pub fn encode_ref(value: &Value, schema: &Schema, buffer: &mut Vec<u8>) {
-    fn encode_ref0(
-        value: &Value,
-        schema: &Schema,
-        buffer: &mut Vec<u8>,
-        schemas_by_name: &mut HashMap<Name, Schema>,
-    ) {
-        match &schema {
-            Schema::Ref { ref name } => {
-                let resolved = schemas_by_name.get(name).unwrap();
-                return encode_ref0(value, resolved, buffer, &mut 
schemas_by_name.clone());
-            }
-            Schema::Record { ref name, .. }
-            | Schema::Enum { ref name, .. }
-            | Schema::Fixed { ref name, .. } => {
-                schemas_by_name.insert(name.clone(), schema.clone());
-            }
-            _ => (),
-        }
+fn encode_internal(
+    value: &Value,
+    schema: &Schema,
+    names: &HashMap<Name, &Schema>,

Review comment:
       Names right now is type aliases to `HashMap<Name, Schema>`. I'll make an 
alias called `NamesRef` for `HashMap<Name, &Schema>`. 




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


Reply via email to