refactor: use list encoder / scheduler directly instead of replicating behavior in map encoder / scheduler #5513
Conversation
… / scheduler that mimics the list behavior
|
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
CC @xloya do you mind reviewing? I don't feel super strongly about this so if you prefer the old approach that is ok too. Still, I think if we can avoid the map encoder having its own logic that will be helpful. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
I think this optimization is great! There's only one issue regarding the list field naming. |
| as Box<dyn StructuralFieldScheduler>) | ||
|
|
||
| let list_field = Field::try_from(ArrowField::new( | ||
| field.name.clone(), |
There was a problem hiding this comment.
Should we use the name of entries_field here?
| } | ||
| let child_decoder = Self::field_to_decoder(entries_field, should_validate)?; | ||
| let list_field = Arc::new(arrow_schema::Field::new( | ||
| field.name().clone(), |
2 participants
This is a minor simplification of the recently added map support. It does not add any functionality, only simplifies the code slightly. The current implementation creates a
StructuralMapEncoderwhich mimics theStructuralListEncoder. This change instead wraps theStructuralListEncoder(composition instead of duplication). As a result we can get rid of theStructuralMapScheduler. The decoder then simply casts from a list array to a map array.Since the list encoder is so simple this isn't really saving us a ton of work. However, if we choose to experiment or add new complicated features to the list encoder in the future then this will help us avoid doing the work twice or missing the changes in the map encoder.