[Variant] Remove ceremony from iterator of variants into VariantArray #8625
Conversation
|
Re: #8611 (comment) I'll throw in the other |
friendlymatthew
force-pushed
the
friendlymatthew/impl-into-iter-variant-array
branch
from
1722f43 to
ece0ffb
Compare
scovich
left a comment
scovich
left a comment
There was a problem hiding this comment.
LGTM!
I'll throw in the other
impl Fromin the second commit
I'm not sure this is needed? VariantArray::from_iterator is pretty clear, and a VariantArray::from wouldn't be able to do anything smarter (can't just take ownership of the vec's data, for example)
| Variant::ShortString(ShortString::try_new("norm").unwrap()), | ||
| ]; | ||
|
|
||
| let variant_array = VariantArray::from_iter(v.into_iter()); |
There was a problem hiding this comment.
Shouldn't need that into_iter call, the trait takes IntoIterator as input?
There was a problem hiding this comment.
Yeah I was just testing something and forgot to revert that. Luckily clippy caught it
| impl<'m, 'v> FromIterator<Option<Variant<'m, 'v>>> for VariantArray { | ||
| fn from_iter<T: IntoIterator<Item = Option<Variant<'m, 'v>>>>(iter: T) -> Self { | ||
| let mut b = VariantArrayBuilder::new(0); | ||
| b.extend(iter); | ||
| b.build() | ||
| } | ||
| } | ||
|
|
||
| impl<'m, 'v> FromIterator<Variant<'m, 'v>> for VariantArray { | ||
| fn from_iter<T: IntoIterator<Item = Variant<'m, 'v>>>(iter: T) -> Self { | ||
| let mut b = VariantArrayBuilder::new(0); | ||
| b.extend(iter); | ||
| b.build() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I wonder if there's a way to get the iterator size without consuming the iterator...
But then again, I haven't ran a profile on the VariantArray code yet. I plan on looking at perf later on
There was a problem hiding this comment.
Most std lib code I've seen relies on the iterator's lower bound hint, with a fallback path if that turned out to be too low.
There's also the ExactSizeIterator trait, but given how rust treats potentially overlapping generic definitions as conflicts, I don't know how to provide implementations for both Iterator and ExactSizeIterator?
Yeah, I agree. I'll just remove the second impl From |
friendlymatthew
force-pushed
the
friendlymatthew/impl-into-iter-variant-array
branch
from
ece0ffb to
eda7457
Compare
friendlymatthew
force-pushed
the
friendlymatthew/impl-into-iter-variant-array
branch
from
eda7457 to
ea264c4
Compare
| } | ||
| } | ||
|
|
||
| impl<'m, 'v> FromIterator<Option<Variant<'m, 'v>>> for VariantArray { |
There was a problem hiding this comment.
If we do this:
| impl<'m, 'v> FromIterator<Option<Variant<'m, 'v>>> for VariantArray { | |
| impl<'m, 'v, V: Into<Variant<'m, 'v>> FromIterator<Option<V>> for VariantArray { |
then we can create a variant array from anything that converts cleanly to variant (e.g. i64 values or &str)?
There was a problem hiding this comment.
Yeah, I thought about this as well, but I figured we'd be creating 2 abstractions?
As in, I wonder if it's better to guide the user to do the Variant::from explicitly 🤔
There was a problem hiding this comment.
This is mostly going to be used by test code, and a homogenous set of e.g. i64 is a pretty common use case. But I don't have strong feelings either way.
There was a problem hiding this comment.
Filed a ticket to track this idea
…tArray` (apache#8627) # Which issue does this PR close? - Closes apache#8610 # Rationale for this change Since the fields of `VariantArray` impl `PartialEq`, this PR simply derives `PartialEq` for `VariantArray` out. Based off of apache#8625
- A follow up from #8625 # Rationale for this change While working on a separate task, I noticed `create_test_variant_array` was redundant. Since `VariantArray` can already be constructed directly from an iterator of Variants, this PR removes the now-unnecessary test helper.
) # Which issue does this PR close? Related-to: #8625 # Rationale for this change 63fe99a introduced the new unit test. I believe the index=3 is a typo. It should be =2. # What changes are included in this PR? Fixed the array index. # Are these changes tested? The fix is in a unit test. # Are there any user-facing changes? No. Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
4 participants
Which issue does this PR close?
VarianttoVariantArray#8606Rationale for this change
Mapping from an iterator of
Variants into aVariantArraycurrently requires a lot of ceremony. This PR abstracts over this.