[Variant] VariantArray field API naming#10124
Conversation
| self.shredding_state.typed_value_column() | ||
| } | ||
|
|
||
| /// Return the [`FieldRef`] of the `metadata` column of the [`StructArray`] |
There was a problem hiding this comment.
Now that I look at it the new _field APIs would only be used for tests in #10015.
// before reuse StructArray::field_by_name (#10110), returns Option<&FieldRef>
let typed_value_field = variant_array.inner().field_by_name("typed_value").unwrap();
// new — the new VariantArray sugar, returns Option<&FieldRef> (no .inner(), no magic string)
let typed_value_field = variant_array.typed_value_field().unwrap();Is it worth introducing for 4 calls in tests?
There was a problem hiding this comment.
I'm curious where metadata_field and value_field will be used, since it seems their fields are fixed? If we don't need them, maybe we can add later when necessary?
There was a problem hiding this comment.
I agree, I think they are not that necessary. typed_value_field may go too.
klion26
left a comment
klion26
left a comment
There was a problem hiding this comment.
LGTM, thanks for the contribution
| self.shredding_state.typed_value_column() | ||
| } | ||
|
|
||
| /// Return the [`FieldRef`] of the `metadata` column of the [`StructArray`] |
There was a problem hiding this comment.
I'm curious where metadata_field and value_field will be used, since it seems their fields are fixed? If we don't need them, maybe we can add later when necessary?
scovich
left a comment
scovich
left a comment
There was a problem hiding this comment.
LGTM, but it sounds like we probably don't need the xxx_field methods (yet)?
Should we update the PR accordingly before merge?
|
@scovich done. |
3 participants
Which issue does this PR close?
VariantArrayfield API naming #10093.Rationale for this change
check issue
What changes are included in this PR?
_fieldAPIs that return&ArrayRefto_column_fieldAPIs that return&FieldRefand tests for themAre these changes tested?
Are there any user-facing changes?