-
Notifications
You must be signed in to change notification settings - Fork 20
Conversation
Similar to `Row`, this adds accessors to `List` and `Map` type. For the former, type-safe accessors are added to get the `i`th element in the list. For the latter, `getKeys` and `getValues` are added to get the map keys and values, respectively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just left a couple of questions.
// TODO: implement `getKeys`, `getValues`, etc., for `Map`. | ||
/// Trait for type-safe access of an index for a `Map` | ||
pub trait MapAccessor { | ||
fn get_keys<'a>(&'a self) -> Box<ListAccessor + 'a>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you consider returning Vec<Field>
or &[Field]
for keys and values? It might result in less code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the question is whether or not it is possible to get struct from map value if we return &[Field]
. In this case we would have to map to Group(Row)
, and then use one of the accessors. So yes, it may be functionally correct to return ListAccessor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we are only exposing Row
, List
and Map
at the moment, and each of them offers accessors. So, here we need to return ListAccessor
instead of list of Field
which is not exposed.
I'm also hesitant on whether the RowIter
should return Box<RowAccessor>
instead of Row
. In future if we implement a batch-oriented record reader, we may want to return a Row
from the current RowBatch
, whose implementation may be different from what we have right now.
Also, it might better to rename the RowAccessor
to Row
, and the current Row
to RowImpl
or something.
Just some random thought 😀 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I would suggest we keep the current implementation for now.
fn get_values<'a>(&'a self) -> Box<ListAccessor + 'a>; | ||
} | ||
|
||
struct MapList<'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will API work if this struct is private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot! Will change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need to export MapList
, but only the accessors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Merged. Thanks @sadikovi for the review! |
Similar to
Row
, this adds accessors toList
andMap
type. For theformer, type-safe accessors are added to get the
i
th element in thelist. For the latter,
getKeys
andgetValues
are added to get the mapkeys and values, respectively.