You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "klion26 (via GitHub)" <gi...@apache.org> on 2025/11/12 15:31:20 UTC
[PR] [Variant] Fix cast logic for Variant to Arrow for DataType::Null [arrow-rs]
klion26 opened a new pull request, #8825:
URL: https://github.com/apache/arrow-rs/pull/8825
# Which issue does this PR close?
- Closes #8810 .
# What changes are included in this PR?
Fix the logic for `VariantToNullArrowRowBuilder` so that it respects the `cast` option
# Are these changes tested?
added test
# Are there any user-facing changes?
Noe
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Fix cast logic for Variant to Arrow for DataType::Null [arrow-rs]
Posted by "liamzwbao (via GitHub)" <gi...@apache.org>.
liamzwbao commented on code in PR #8825:
URL: https://github.com/apache/arrow-rs/pull/8825#discussion_r2539907329
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1039,6 +1040,43 @@ mod test {
arrow::array::NullArray::new(3)
);
+ perfectly_shredded_variant_array_fn!(perfectly_shredded_null_variant_array_with_int, || {
+ Int32Array::from(vec![Some(32), Some(64), Some(48)])
+ });
+
+ // We append null values if type miss match happens in safe mode
+ perfectly_shredded_to_arrow_primitive_test!(
+ get_variant_perfectly_shredded_null_with_type_missmatch_in_safe_mode,
+ DataType::Null,
+ perfectly_shredded_null_variant_array_with_int,
+ arrow::array::NullArray::new(3)
+ );
+
+ // We'll return an error if type miss match happens in strict mode
+ #[test]
+ fn get_variant_perfectly_shredded_null_as_null_with_type_missmatch_in_strict_mode() {
+ let array = perfectly_shredded_null_variant_array_with_int();
+ let field = Field::new("typed_value", DataType::Null, true);
+ let options = GetOptions::new()
+ .with_as_type(Some(FieldRef::from(field)))
+ .with_cast_options(CastOptions {
Review Comment:
I remember we had similar discussion before. I vote for unifying them, and we don't really need the `format_options` in the `arrow::compute::CastOptions`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Fix cast logic for Variant to Arrow for DataType::Null [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8825:
URL: https://github.com/apache/arrow-rs/pull/8825#discussion_r2537898428
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -696,26 +696,43 @@ impl VariantToBinaryVariantArrowRowBuilder {
}
}
-struct FakeNullBuilder(NullArray);
+pub(crate) struct VariantToNullArrowRowBuilder<'a> {
+ cast_option: &'a CastOptions<'a>,
+ item_count: usize,
+}
-impl FakeNullBuilder {
- fn new(capacity: usize) -> Self {
- Self(NullArray::new(capacity))
+impl<'a> VariantToNullArrowRowBuilder<'a> {
+ fn new(cast_option: &'a CastOptions<'a>) -> Self {
+ Self {
+ cast_option,
+ item_count: 0,
+ }
}
- fn append_value<T>(&mut self, _: T) {}
- fn append_null(&mut self) {}
- fn finish(self) -> NullArray {
- self.0
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ if value.as_null().is_some() {
+ self.item_count += 1;
+ Ok(true)
+ } else if self.cast_option.safe {
+ self.append_null()?;
+ Ok(false)
+ } else {
+ Err(ArrowError::CastError(format!(
Review Comment:
Yes, it can reuse the `define_variant_to_primitive_builder!` macro, updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Fix cast logic for Variant to Arrow for DataType::Null [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8825:
URL: https://github.com/apache/arrow-rs/pull/8825#discussion_r2539310678
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -696,24 +696,32 @@ impl VariantToBinaryVariantArrowRowBuilder {
}
}
-struct FakeNullBuilder(NullArray);
+struct FakeNullBuilder {
+ item_count: usize,
+}
impl FakeNullBuilder {
- fn new(capacity: usize) -> Self {
- Self(NullArray::new(capacity))
+ fn new() -> Self {
+ Self { item_count: 0 }
+ }
Review Comment:
tiny nit: Consider `#[derive(Default)]` and `FakeNullBuilder::default()` at L723 below?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Fix cast logic for Variant to Arrow for DataType::Null [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8825:
URL: https://github.com/apache/arrow-rs/pull/8825
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Fix cast logic for Variant to Arrow for DataType::Null [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8825:
URL: https://github.com/apache/arrow-rs/pull/8825#discussion_r2541684099
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1039,6 +1040,43 @@ mod test {
arrow::array::NullArray::new(3)
);
+ perfectly_shredded_variant_array_fn!(perfectly_shredded_null_variant_array_with_int, || {
+ Int32Array::from(vec![Some(32), Some(64), Some(48)])
+ });
+
+ // We append null values if type miss match happens in safe mode
+ perfectly_shredded_to_arrow_primitive_test!(
+ get_variant_perfectly_shredded_null_with_type_missmatch_in_safe_mode,
+ DataType::Null,
+ perfectly_shredded_null_variant_array_with_int,
+ arrow::array::NullArray::new(3)
+ );
+
+ // We'll return an error if type miss match happens in strict mode
+ #[test]
+ fn get_variant_perfectly_shredded_null_as_null_with_type_missmatch_in_strict_mode() {
+ let array = perfectly_shredded_null_variant_array_with_int();
+ let field = Field::new("typed_value", DataType::Null, true);
+ let options = GetOptions::new()
+ .with_as_type(Some(FieldRef::from(field)))
+ .with_cast_options(CastOptions {
Review Comment:
Will file an issue to track this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Fix cast logic for Variant to Arrow for DataType::Null [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8825:
URL: https://github.com/apache/arrow-rs/pull/8825#discussion_r2518703496
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -678,23 +678,32 @@ impl VariantToBinaryVariantArrowRowBuilder {
}
}
-struct FakeNullBuilder(NullArray);
+pub(crate) struct VariantToNullArrowRowBuilder {
+ capacity: usize,
+}
-impl FakeNullBuilder {
+impl VariantToNullArrowRowBuilder {
fn new(capacity: usize) -> Self {
- Self(NullArray::new(capacity))
+ Self { capacity }
}
- fn append_value<T>(&mut self, _: T) {}
- fn append_null(&mut self) {}
- fn finish(self) -> NullArray {
- self.0
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ if let Some(_) = value.as_null() {
+ Ok(true)
+ } else {
+ // Null type only accepts nulls
Review Comment:
Choose to return err because `Arrays of type Null cannot contain a null bitmask`. If we return `NullArray::new(valid_value_count)`, then the length of the result may not be the same as the input(the other types will add `None` in the result)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Fix cast logic for Variant to Arrow for DataType::Null [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8825:
URL: https://github.com/apache/arrow-rs/pull/8825#discussion_r2541682755
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -696,24 +696,32 @@ impl VariantToBinaryVariantArrowRowBuilder {
}
}
-struct FakeNullBuilder(NullArray);
+struct FakeNullBuilder {
+ item_count: usize,
+}
impl FakeNullBuilder {
- fn new(capacity: usize) -> Self {
- Self(NullArray::new(capacity))
+ fn new() -> Self {
+ Self { item_count: 0 }
+ }
Review Comment:
Thanks for the suggestion, it's more elegant, added in the new commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Fix cast logic for Variant to Arrow for DataType::Null [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8825:
URL: https://github.com/apache/arrow-rs/pull/8825#discussion_r2541684099
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1039,6 +1040,43 @@ mod test {
arrow::array::NullArray::new(3)
);
+ perfectly_shredded_variant_array_fn!(perfectly_shredded_null_variant_array_with_int, || {
+ Int32Array::from(vec![Some(32), Some(64), Some(48)])
+ });
+
+ // We append null values if type miss match happens in safe mode
+ perfectly_shredded_to_arrow_primitive_test!(
+ get_variant_perfectly_shredded_null_with_type_missmatch_in_safe_mode,
+ DataType::Null,
+ perfectly_shredded_null_variant_array_with_int,
+ arrow::array::NullArray::new(3)
+ );
+
+ // We'll return an error if type miss match happens in strict mode
+ #[test]
+ fn get_variant_perfectly_shredded_null_as_null_with_type_missmatch_in_strict_mode() {
+ let array = perfectly_shredded_null_variant_array_with_int();
+ let field = Field::new("typed_value", DataType::Null, true);
+ let options = GetOptions::new()
+ .with_as_type(Some(FieldRef::from(field)))
+ .with_cast_options(CastOptions {
Review Comment:
~~Will file an issue to track this~~
Filed an issue #8873
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Fix cast logic for Variant to Arrow for DataType::Null [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8825:
URL: https://github.com/apache/arrow-rs/pull/8825#discussion_r2523588384
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -678,23 +678,32 @@ impl VariantToBinaryVariantArrowRowBuilder {
}
}
-struct FakeNullBuilder(NullArray);
+pub(crate) struct VariantToNullArrowRowBuilder {
+ capacity: usize,
+}
-impl FakeNullBuilder {
+impl VariantToNullArrowRowBuilder {
fn new(capacity: usize) -> Self {
- Self(NullArray::new(capacity))
+ Self { capacity }
}
- fn append_value<T>(&mut self, _: T) {}
- fn append_null(&mut self) {}
- fn finish(self) -> NullArray {
- self.0
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ if let Some(_) = value.as_null() {
+ Ok(true)
+ } else {
+ // Null type only accepts nulls
Review Comment:
The comment in the issue about counting appended values/nulls was kind of unrelated: _technically_ there's no guarantee the number of appends matches the initial capacity estimate, which in arrow API is only an estimate. So we should ideally have both `append_null` and `append_value` increment some counter, whose final value dictates the size of the final array we create.
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -678,23 +678,32 @@ impl VariantToBinaryVariantArrowRowBuilder {
}
}
-struct FakeNullBuilder(NullArray);
+pub(crate) struct VariantToNullArrowRowBuilder {
+ capacity: usize,
+}
-impl FakeNullBuilder {
+impl VariantToNullArrowRowBuilder {
fn new(capacity: usize) -> Self {
- Self(NullArray::new(capacity))
+ Self { capacity }
}
- fn append_value<T>(&mut self, _: T) {}
- fn append_null(&mut self) {}
- fn finish(self) -> NullArray {
- self.0
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ if let Some(_) = value.as_null() {
+ Ok(true)
+ } else {
+ // Null type only accepts nulls
Review Comment:
Agree we can't add a NULL mask to a NullArray. But I thought the idea was to let the normal strict-mode checking either error out or call (our fake) `append_null` that actually does the same thing as `append_value` (namely nothing). No null masks in sight.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Fix cast logic for Variant to Arrow for DataType::Null [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on PR #8825:
URL: https://github.com/apache/arrow-rs/pull/8825#issuecomment-3540248411
@scovich Thanks for the review and sorry for the late reply; I had to handle some unforeseen circumstances last week.
I've addressed the comments in the new commit. Please take another look.
In the last commit, I assumed that before `append_null` here, it can't be accepted as the target type is `Null`(we can't distinguish the target type and the missing element), but it seems `append_null` when `value.as_null.is_none()` in safe mode is fine(we may don't need to distinguish the two types, and there is no null mask in NullArray).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Fix cast logic for Variant to Arrow for DataType::Null [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8825:
URL: https://github.com/apache/arrow-rs/pull/8825#discussion_r2535607866
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -696,26 +696,43 @@ impl VariantToBinaryVariantArrowRowBuilder {
}
}
-struct FakeNullBuilder(NullArray);
+pub(crate) struct VariantToNullArrowRowBuilder<'a> {
+ cast_option: &'a CastOptions<'a>,
+ item_count: usize,
+}
-impl FakeNullBuilder {
- fn new(capacity: usize) -> Self {
- Self(NullArray::new(capacity))
+impl<'a> VariantToNullArrowRowBuilder<'a> {
+ fn new(cast_option: &'a CastOptions<'a>) -> Self {
+ Self {
+ cast_option,
+ item_count: 0,
+ }
}
- fn append_value<T>(&mut self, _: T) {}
- fn append_null(&mut self) {}
- fn finish(self) -> NullArray {
- self.0
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ if value.as_null().is_some() {
+ self.item_count += 1;
+ Ok(true)
+ } else if self.cast_option.safe {
+ self.append_null()?;
+ Ok(false)
+ } else {
+ Err(ArrowError::CastError(format!(
Review Comment:
`define_variant_to_primitive_builder!` macro handles all this already?
Why not use it?
```rust
|value| matches!(value, Variant::Null).then_some(0)
```
and then
```rust
fn append_value(&mut self, _dummy: i32) {
self.item_count += 1;
}
fn append_null(&mut self) {
self.item_count += 1;
}
```
(no need to capture the cast options, the macro handles that part)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Fix cast logic for Variant to Arrow for DataType::Null [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on PR #8825:
URL: https://github.com/apache/arrow-rs/pull/8825#issuecomment-3547326324
@scovich I've updated the code. Please take another look, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Fix cast logic for Variant to Arrow for DataType::Null [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8825:
URL: https://github.com/apache/arrow-rs/pull/8825#discussion_r2533010768
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1039,6 +1040,43 @@ mod test {
arrow::array::NullArray::new(3)
);
+ perfectly_shredded_variant_array_fn!(perfectly_shredded_null_variant_array_with_int, || {
+ Int32Array::from(vec![Some(32), Some(64), Some(48)])
+ });
+
+ // We append null values if type miss match happens in safe mode
+ perfectly_shredded_to_arrow_primitive_test!(
+ get_variant_perfectly_shredded_null_with_type_missmatch_in_safe_mode,
+ DataType::Null,
+ perfectly_shredded_null_variant_array_with_int,
+ arrow::array::NullArray::new(3)
+ );
+
+ // We'll return an error if type miss match happens in strict mode
+ #[test]
+ fn get_variant_perfectly_shredded_null_as_null_with_type_missmatch_in_strict_mode() {
+ let array = perfectly_shredded_null_variant_array_with_int();
+ let field = Field::new("typed_value", DataType::Null, true);
+ let options = GetOptions::new()
+ .with_as_type(Some(FieldRef::from(field)))
+ .with_cast_options(CastOptions {
Review Comment:
We use `arrow::compute::CastOptions` in `variant_get` and `crate::CastOptions` in `cast_to_variant_with_options`, not sure if we need to unify these in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Fix cast logic for Variant to Arrow for DataType::Null [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on PR #8825:
URL: https://github.com/apache/arrow-rs/pull/8825#issuecomment-3552278189
@alamb Please take a final review when you're free, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org