Conversation
| Variant, VariantBuilder, VariantDecimal16, VariantDecimal4, VariantDecimal8, | ||
| }; | ||
|
|
||
| fn convert_timestamp( |
There was a problem hiding this comment.
Move this helper function below cast_to_variant so that the public function appears first, making this file easier to read and reason about.
| fn convert_list<O: OffsetSizeTrait>( | ||
| input: &dyn Array, | ||
| builder: &mut VariantArrayBuilder, | ||
| ) -> Result<(), ArrowError> { |
There was a problem hiding this comment.
The logic for List and LargeList is the same, refactor them into this generic helper function
| Ok(()) | ||
| } | ||
|
|
||
| fn convert_struct(input: &dyn Array, builder: &mut VariantArrayBuilder) -> Result<(), ArrowError> { |
There was a problem hiding this comment.
moved from the Struct branch without any changes
| // Add all values from the slice | ||
| for j in start..end { | ||
| list_builder.append_value(values_variant_array.value(j)); | ||
| fn convert_map( |
There was a problem hiding this comment.
moved from the Map branch without any changes
|
|
||
| #[test] | ||
| fn test_cast_map_with_non_string_keys_to_variant_object() { | ||
| fn test_cast_to_variant_map_with_non_string_keys() { |
There was a problem hiding this comment.
rename to match the naming convention in this file
Thanks for pointing out! I can go either way as that PR only touches the timestamp conversion, but it might be better to make this one go first |
|
@alamb -- I think this one is ready for your review+merge; two other PR are stacked on top waiting for it: |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @liamzwbao and @scovich
|
Take care and we'll see you when you get back |
3 participants
Which issue does this PR close?
cast_to_variant#8234.Rationale for this change
What changes are included in this PR?
Are these changes tested?
Covered by existing tests
Are there any user-facing changes?
N/A