Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add lookup tables to StructType #12

Merged
merged 10 commits into from
Jul 28, 2023
90 changes: 83 additions & 7 deletions src/spec/datatypes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,14 @@
/*!
* Data Types
*/
use std::{fmt, ops::Index};
use std::{
collections::{BTreeMap, HashMap},
fmt,
ops::Index,
};

use serde::{
de::{Error, IntoDeserializer},
de::{self, Error, IntoDeserializer, MapAccess, Visitor},
Deserialize, Deserializer, Serialize, Serializer,
};

Expand Down Expand Up @@ -190,21 +194,87 @@ impl fmt::Display for PrimitiveType {
}

/// DataType for a specific struct
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone)]
#[derive(Debug, Serialize, PartialEq, Eq, Clone)]
#[serde(rename = "struct", tag = "type")]
pub struct StructType {
/// Struct fields
fields: Vec<StructField>,
/// Lookup for index by field id
#[serde(skip_serializing)]
Copy link
Member

Choose a reason for hiding this comment

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

I have to say this is why I want an in-memory represents 😢. Since we have already reached a consensus in the previous discussion, I will not initiate a new round here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't want to force this serialization/deserialization design. If we don't have a consensus on the design, I think we should get back to the discussion. It doesn't make sense to merge something that might get changed later on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Per our discussion, if we want to avoid such annotation, we could implement a serializer for StructType.

Copy link
Member

Choose a reason for hiding this comment

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

if we want to avoid such annotation, we could implement a serializer for StructType.

Currently, it appears that there is no immediate need for this. Let's proceed and observe how the situation develops.

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, it appears that there is no immediate need for this. Let's proceed and observe how the situation develops.

Totally agree, for simple cases like this, it doesn't make code more difficult to read, so we can go on with it.

id_lookup: BTreeMap<i32, usize>,
/// Lookup for index by field name
#[serde(skip_serializing)]
name_lookup: HashMap<String, usize>,
}

impl<'de> Deserialize<'de> for StructType {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
#[derive(Deserialize)]
#[serde(field_identifier, rename_all = "lowercase")]
enum Field {
Type,
Fields,
}

struct StructTypeVisitor;

impl<'de> Visitor<'de> for StructTypeVisitor {
type Value = StructType;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str("struct")
}

fn visit_map<V>(self, mut map: V) -> Result<StructType, V::Error>
where
V: MapAccess<'de>,
{
let mut fields = None;
while let Some(key) = map.next_key()? {
match key {
Field::Type => (),
Field::Fields => {
if fields.is_some() {
return Err(de::Error::duplicate_field("fields"));
}
fields = Some(map.next_value()?);
}
}
}
let fields: Vec<StructField> =
fields.ok_or_else(|| de::Error::missing_field("fields"))?;

Ok(StructType::new(fields))
}
}

const FIELDS: &[&str] = &["type", "fields"];
deserializer.deserialize_struct("struct", FIELDS, StructTypeVisitor)
}
}

impl StructType {
///
JanKaul marked this conversation as resolved.
Show resolved Hide resolved
pub fn new(fields: Vec<StructField>) -> Self {
let id_lookup = BTreeMap::from_iter(fields.iter().enumerate().map(|(i, x)| (x.id, i)));
let name_lookup =
HashMap::from_iter(fields.iter().enumerate().map(|(i, x)| (x.name.clone(), i)));
Self {
fields,
id_lookup,
name_lookup,
JanKaul marked this conversation as resolved.
Show resolved Hide resolved
}
}
/// Get structfield with certain id
pub fn get(&self, id: usize) -> Option<&StructField> {
self.fields.iter().find(|field| field.id as usize == id)
pub fn get(&self, id: i32) -> Option<&StructField> {
JanKaul marked this conversation as resolved.
Show resolved Hide resolved
self.fields.get(*self.id_lookup.get(&id)?)
}
/// Get structfield with certain name
pub fn get_name(&self, name: &str) -> Option<&StructField> {
self.fields.iter().find(|field| field.name == name)
pub fn get_by_name(&self, name: &str) -> Option<&StructField> {
self.fields.get(*self.name_lookup.get(name)?)
}
}

Expand Down Expand Up @@ -349,6 +419,8 @@ mod tests {
initial_default: None,
write_default: None,
}],
id_lookup: BTreeMap::from([(1, 0)]),
name_lookup: HashMap::from([("id".to_string(), 0)]),
}),
)
}
Expand Down Expand Up @@ -381,6 +453,8 @@ mod tests {
initial_default: None,
write_default: None,
}],
id_lookup: BTreeMap::from([(1, 0)]),
name_lookup: HashMap::from([("id".to_string(), 0)]),
}),
)
}
Expand Down Expand Up @@ -431,6 +505,8 @@ mod tests {
write_default: None,
},
],
id_lookup: BTreeMap::from([(1, 0), (2, 1)]),
name_lookup: HashMap::from([("id".to_string(), 0), ("data".to_string(), 1)]),
}),
)
}
Expand Down