Implement support for list of Dictionaries #664
Conversation
gsilvestrin
commented
Mar 8, 2023
•
gsilvestrin
commented
- Fixed a bug in write_manifest that would skip writing the dict values for the first field in the schema
- Fixed a bug in write_manifest that would skip writing the dict values for the first field in the schema
gsilvestrin
force-pushed
the
gsilvestrin/lists_dictionary
branch
from
35574ce to
4c3c350
Compare
gsilvestrin
changed the title
rust/src/datatypes.rs
Outdated
| _ => { | ||
| // Add list / large list support. | ||
| // Add list / large list support. - should we panic? |
There was a problem hiding this comment.
Are there more types to implement?
There was a problem hiding this comment.
There are more types. We dont need to support them yet.
Can panic or return error here.
There was a problem hiding this comment.
panic causes more issues - this method is called for all fields, not only dictionaries. So we would need to either make this match exhaustive or keep the comment (I left the comment)
| @@ -41,7 +41,7 @@ pub async fn write_manifest( | |||
| ) -> Result<usize> { | |||
| // Write dictionary values. | |||
| let max_field_id = manifest.schema.max_field_id().unwrap_or(-1); | |||
| for field_id in 1..max_field_id + 1 { | |||
| for field_id in 0..max_field_id + 1 { | |||
There was a problem hiding this comment.
Could potentially be (0..max_field_id), but mut_field_by_id will return None when the id does not exist
rust/src/datatypes.rs
Outdated
| @@ -375,8 +377,16 @@ impl Field { | |||
| lance_field.set_dictionary(struct_arr.column(i)); | |||
| } | |||
| } | |||
| DataType::List(_) => { | |||
| let list_arr = arr.as_any().downcast_ref::<ListArray>().unwrap(); | |||
There was a problem hiding this comment.
is there a as_list_array() in arrow-rs?
There was a problem hiding this comment.
Yes, I refactored it
rust/src/datatypes.rs
Outdated
| _ => { | ||
| // Add list / large list support. | ||
| // Add list / large list support. - should we panic? |
There was a problem hiding this comment.
There are more types. We dont need to support them yet.
Can panic or return error here.
