-
Notifications
You must be signed in to change notification settings - Fork 224
Added FixedSizeListScalar
and FixedSizeBinaryScalar
#786
Added FixedSizeListScalar
and FixedSizeBinaryScalar
#786
Conversation
…feature/fixedsize-list-and-binary-scalar
Codecov Report
@@ Coverage Diff @@
## main #786 +/- ##
=======================================
Coverage 71.08% 71.08%
=======================================
Files 317 319 +2
Lines 16623 16664 +41
=======================================
+ Hits 11816 11846 +30
- Misses 4807 4818 +11
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome! I left a small number of comments / suggestions. Awesome PR, @illumination-k !
src/scalar/fixed_size_list.rs
Outdated
values: Arc<dyn Array>, | ||
is_valid: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about
values: Arc<dyn Array>, | |
is_valid: bool, | |
values: Option<Arc<dyn Array>>, |
and remove is_valid
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I referred to ListScalar
, but I think Option<Arc<dyn Array>>
is better and value method should return Option<&Arc<dyn Array>>
for consistency of other scalar.
StructScalar
and ListScalar
also have is_valid
. Should we use Option
and remove is_valid
for them? (Maybe another PR)
FixedSizeListScalar
and FixedSizeBinaryScalar
FixedSizeListScalar
and FixedSizeBinaryScalar
Thanks a lot, @illumination-k ! |
FixedSizeListScalar
andFixedSizeBinaryScalar
FixedSizeListScalar
andFixedSizeBinaryScalar
innew_scalar
related to #782 and #783