feat: Add IndexByName and IndexById to Namemapping
#1299
Conversation
jdockerty
left a comment
jdockerty
left a comment
There was a problem hiding this comment.
A question and a clarification 💯
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @jonathanc-n , just finished first round of review, generally looks good.
|
@jdockerty @liurenjie1024 Thanks for the reviews. Should be fixed now! |
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
Thank you @jonathanc-n for working on this and thank you @liurenjie1024 for the review.
Wait for @liurenjie1024 for another look.
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @jonathanc-n for this pr, just finished second round of review. There are still some problems to fix.
| } | ||
|
|
||
| /// Recursively visits the entire name mapping using visitor | ||
| fn visit_name_mapping<V>(namespace: &Vec<Arc<MappedField>>, visitor: &V) -> V::S |
There was a problem hiding this comment.
| fn visit_name_mapping<V>(namespace: &Vec<Arc<MappedField>>, visitor: &V) -> V::S | |
| fn visit_name_mapping<V>(namespace: &NameMapping, visitor: &V) -> V::S |
It's odd that the name is visit_name_mapping while accepts a list of fields.
There was a problem hiding this comment.
Also the return value should be a Result
There was a problem hiding this comment.
This is because when creating a new NameMapping we call visit_schema which creates an array of mapped fields which are then called on by index_by_id + index_by_name
There was a problem hiding this comment.
@liurenjie1024 This should be good for another look, thanks!
| pub trait NameMappingVisitor { | ||
| /// Aggregated result of `MappedField`s | ||
| type S; | ||
|
|
||
| /// Handles entire `NameMapping` field | ||
| fn mapping(&self, field_result: Self::S) -> Self::S; | ||
|
|
||
| /// Handles a single `MappedField` | ||
| fn field(&self, field: &MappedField, field_result: Self::S) -> Self::S; | ||
| } |
There was a problem hiding this comment.
| pub trait NameMappingVisitor { | |
| /// Aggregated result of `MappedField`s | |
| type S; | |
| /// Handles entire `NameMapping` field | |
| fn mapping(&self, field_result: Self::S) -> Self::S; | |
| /// Handles a single `MappedField` | |
| fn field(&self, field: &MappedField, field_result: Self::S) -> Self::S; | |
| } | |
| pub trait NameMappingVisitor { | |
| type T; | |
| /// Aggregated result of `MappedField`s | |
| type S; | |
| /// Handles entire `NameMapping` field | |
| fn mapping(&mut self, mapping: &NameMapping, field_result: Vec<Self::T>) -> Result<Self::S>; | |
| /// Handles a single `MappedField` | |
| fn field(&mut self, field: &MappedField, field_result: Vec<Self::T>) -> Result<Self::T>; | |
| } |
| fn visit_name_mapping<V>(name_mapping: &Vec<Arc<MappedField>>, visitor: &V) -> Result<V::S> | ||
| where | ||
| V: NameMappingVisitor, | ||
| V::S: IntoIterator + FromIterator<<V::S as IntoIterator>::Item>, | ||
| { | ||
| let root_result = visit_fields(name_mapping, visitor); | ||
| Ok(visitor.mapping(root_result)) | ||
| } | ||
|
|
||
| /// Recursively visits a slice of mapped fields using visitor | ||
| fn visit_fields<V>(fields: &Vec<Arc<MappedField>>, visitor: &V) -> V::S | ||
| where | ||
| V: NameMappingVisitor, | ||
| V::S: IntoIterator + FromIterator<<V::S as IntoIterator>::Item>, |
There was a problem hiding this comment.
Please refine the visiting logic with suggested visitor.
| /// Creates new `NameMapping` instance | ||
| pub fn new(schema: &Schema) -> Self { | ||
| // Create `MappedField` instances by visiting schema. | ||
| let root = visit_schema(schema, &mut CreateMapping).unwrap(); |
There was a problem hiding this comment.
We can construct a NameMapping without index here, and then assign the indcies to it before returning.
4 participants
Which issue does this PR close?
Build on #1116
What changes are included in this PR?
Adds IndexByName and index by id to name mapping
Are these changes tested?
Yes, unit tests