You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "raghav-reglobe (via GitHub)" <gi...@apache.org> on 2026/08/09 08:48:57 UTC
[PR] Error instead of silent `Variant::Null` for unimplemented `typed_value` types [arrow-rs]
raghav-reglobe opened a new pull request, #10598:
URL: https://github.com/apache/arrow-rs/pull/10598
# Which issue does this PR close?
- Closes #10597.
# Rationale for this change
`typed_value_to_variant`'s fallback arm for unimplemented `typed_value` types was `debug_assert!(false)` + `Ok(Variant::Null)`. Release builds compile out the assert, so reading a row whose `typed_value` is valid but of an unimplemented type (e.g. an object-shredded `Struct`) silently misreported the stored value as `Variant::Null` — data that exists reads as null with no signal, while the same read panics in debug builds.
# What changes are included in this PR?
- The fallback arm returns `ArrowError::NotYetImplemented` instead — consistent across debug/release, and within `try_value`'s documented error contract ("Errors if the data in `typed_value` cannot be interpreted as a valid `Variant`").
- A regression test pinning that an object-shredded `typed_value` errors rather than returning `Variant::Null`.
Implementing the remaining conversions (objects/lists) is left for a follow-up — the borrowed `Variant<'_, '_>` return can't express an assembled object without owned bytes, so that is a larger change.
# Are there any user-facing changes?
`VariantArray::try_value` now returns an error (and `value` panics in release as it already did in debug) for unimplemented `typed_value` types, instead of silently returning `Variant::Null` in release builds.
--
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] Error instead of silent `Variant::Null` for unimplemented `typed_value` types [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #10598:
URL: https://github.com/apache/arrow-rs/pull/10598#discussion_r3749464028
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1152,17 +1152,13 @@ fn typed_value_to_variant(typed_value: &ArrayRef, index: usize) -> Result<Varian
}
// todo other types here (note this is very similar to cast_to_variant.rs)
// so it would be great to figure out how to share this code
- _ => {
- // We shouldn't panic in production code, but this is a
- // placeholder until we implement more types
- // https://github.com/apache/arrow-rs/issues/8091
- debug_assert!(
- false,
- "Unsupported typed_value type: {}",
- typed_value.data_type()
- );
- Ok(Variant::Null)
- }
+ //
+ // Returning Variant::Null here would silently misreport stored values
+ // as null in release builds (https://github.com/apache/arrow-rs/issues/10597)
+ _ => Err(ArrowError::NotYetImplemented(format!(
Review Comment:
Checked the code here, the type(`DataType::Struct`) is valid here, it needs to be implemented.
--
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] Error instead of silent `Variant::Null` for unimplemented `typed_value` types [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #10598:
URL: https://github.com/apache/arrow-rs/pull/10598#discussion_r3752409423
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1152,17 +1152,13 @@ fn typed_value_to_variant(typed_value: &ArrayRef, index: usize) -> Result<Varian
}
// todo other types here (note this is very similar to cast_to_variant.rs)
// so it would be great to figure out how to share this code
- _ => {
- // We shouldn't panic in production code, but this is a
- // placeholder until we implement more types
- // https://github.com/apache/arrow-rs/issues/8091
- debug_assert!(
- false,
- "Unsupported typed_value type: {}",
- typed_value.data_type()
- );
- Ok(Variant::Null)
- }
+ //
+ // Returning Variant::Null here would silently misreport stored values
+ // as null in release builds (https://github.com/apache/arrow-rs/issues/10597)
+ _ => Err(ArrowError::NotYetImplemented(format!(
+ "VariantArray does not yet support typed_value type: {}",
+ typed_value.data_type()
+ ))),
Review Comment:
These types being composite make `try_value`/`value` returning borrowed Variant for them painful.
We should leave a suggestion to unshred the array before extracting the value for now and decide if we want to proceed with this or defer.
```suggestion
//
// Composite shredded values may require combining `value` and
// `typed_value` and allocating new encoded bytes. `try_value` returns
// borrowed Variant, so callers must unshred the array first.
_ => Err(ArrowError::NotYetImplemented(format!(
"VariantArray::try_value cannot materialize typed_value of type {} \
as a borrowed Variant; call unshred_variant first",
typed_value.data_type()
))),
```
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1152,17 +1152,13 @@ fn typed_value_to_variant(typed_value: &ArrayRef, index: usize) -> Result<Varian
}
// todo other types here (note this is very similar to cast_to_variant.rs)
// so it would be great to figure out how to share this code
- _ => {
- // We shouldn't panic in production code, but this is a
- // placeholder until we implement more types
- // https://github.com/apache/arrow-rs/issues/8091
- debug_assert!(
- false,
- "Unsupported typed_value type: {}",
- typed_value.data_type()
- );
- Ok(Variant::Null)
- }
+ //
+ // Returning Variant::Null here would silently misreport stored values
+ // as null in release builds (https://github.com/apache/arrow-rs/issues/10597)
+ _ => Err(ArrowError::NotYetImplemented(format!(
Review Comment:
@raghav-reglobe thanks for catching this. I think I was working on smth related to this earlier, but it was not documented in an issue.
This PR can close the bug of incorrectly returning Null for unsupported types. I will create a separate issue to track supporting composite types for `typed_value_to_variant`
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1152,17 +1152,13 @@ fn typed_value_to_variant(typed_value: &ArrayRef, index: usize) -> Result<Varian
}
// todo other types here (note this is very similar to cast_to_variant.rs)
// so it would be great to figure out how to share this code
- _ => {
- // We shouldn't panic in production code, but this is a
- // placeholder until we implement more types
- // https://github.com/apache/arrow-rs/issues/8091
- debug_assert!(
- false,
- "Unsupported typed_value type: {}",
- typed_value.data_type()
- );
- Ok(Variant::Null)
- }
+ //
+ // Returning Variant::Null here would silently misreport stored values
+ // as null in release builds (https://github.com/apache/arrow-rs/issues/10597)
Review Comment:
We shouldn't add a timeline narrating comment about what used to be there.
--
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] Error instead of silent `Variant::Null` for unimplemented `typed_value` types [arrow-rs]
Posted by "raghav-reglobe (via GitHub)" <gi...@apache.org>.
raghav-reglobe commented on code in PR #10598:
URL: https://github.com/apache/arrow-rs/pull/10598#discussion_r3744780189
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1152,17 +1152,13 @@ fn typed_value_to_variant(typed_value: &ArrayRef, index: usize) -> Result<Varian
}
// todo other types here (note this is very similar to cast_to_variant.rs)
// so it would be great to figure out how to share this code
- _ => {
- // We shouldn't panic in production code, but this is a
- // placeholder until we implement more types
- // https://github.com/apache/arrow-rs/issues/8091
- debug_assert!(
- false,
- "Unsupported typed_value type: {}",
- typed_value.data_type()
- );
- Ok(Variant::Null)
- }
+ //
+ // Returning Variant::Null here would silently misreport stored values
+ // as null in release builds (https://github.com/apache/arrow-rs/issues/10597)
+ _ => Err(ArrowError::NotYetImplemented(format!(
Review Comment:
Still to implement: shredded objects (`Struct`) and shredded arrays (the `List` family) — both legal per the shredding spec, and reachable today since `shred_variant` itself produces them (the new test builds the `Struct` case that way). Truly-illegal types can't reach this arm: `VariantArray::try_new` already rejects them at construction via `canonicalize_and_verify_data_type` (`Illegal shredded value type`, `InvalidArgumentError`). So by the time `typed_value_to_variant` runs, the type is spec-legal and the only gap is the missing conversion — which is why `NotYetImplemented` seemed like the accurate classification. Implementing object/list here needs owned bytes (the borrowed `Variant<'_, '_>` return can't express an assembled object), so I left that for a follow-up.
--
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] Error instead of silent `Variant::Null` for unimplemented `typed_value` types [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #10598:
URL: https://github.com/apache/arrow-rs/pull/10598#discussion_r3775659238
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1152,17 +1152,13 @@ fn typed_value_to_variant(typed_value: &ArrayRef, index: usize) -> Result<Varian
}
// todo other types here (note this is very similar to cast_to_variant.rs)
// so it would be great to figure out how to share this code
- _ => {
- // We shouldn't panic in production code, but this is a
- // placeholder until we implement more types
- // https://github.com/apache/arrow-rs/issues/8091
- debug_assert!(
- false,
- "Unsupported typed_value type: {}",
- typed_value.data_type()
- );
- Ok(Variant::Null)
- }
+ //
+ // Returning Variant::Null here would silently misreport stored values
+ // as null in release builds (https://github.com/apache/arrow-rs/issues/10597)
+ _ => Err(ArrowError::NotYetImplemented(format!(
Review Comment:
The issue - #10620
--
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] Error instead of silent `Variant::Null` for unimplemented `typed_value` types [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #10598:
URL: https://github.com/apache/arrow-rs/pull/10598#issuecomment-5292595333
thanks @raghav-reglobe, @klion26 & @sdf-jkl
--
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] Error instead of silent `Variant::Null` for unimplemented `typed_value` types [arrow-rs]
Posted by "raghav-reglobe (via GitHub)" <gi...@apache.org>.
raghav-reglobe commented on PR #10598:
URL: https://github.com/apache/arrow-rs/pull/10598#issuecomment-5233085877
Still to implement: shredded objects (`Struct`) and shredded arrays (the `List` family) — both legal per the shredding spec, and reachable today since `shred_variant` itself produces them (the new test builds the `Struct` case that way). Truly-illegal types can't reach this arm: `VariantArray::try_new` already rejects them at construction via `canonicalize_and_verify_data_type` (`Illegal shredded value type`, `InvalidArgumentError`). So by the time `typed_value_to_variant` runs, the type is spec-legal and the only gap is the missing conversion — which is why `NotYetImplemented` seemed like the accurate classification. Implementing object/list here needs owned bytes (the borrowed `Variant<'_, '_>` return can't express an assembled object), so I left that for a follow-up.
--
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] Error instead of silent `Variant::Null` for unimplemented `typed_value` types [arrow-rs]
Posted by "raghav-reglobe (via GitHub)" <gi...@apache.org>.
raghav-reglobe commented on code in PR #10598:
URL: https://github.com/apache/arrow-rs/pull/10598#discussion_r3775571913
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1152,17 +1152,13 @@ fn typed_value_to_variant(typed_value: &ArrayRef, index: usize) -> Result<Varian
}
// todo other types here (note this is very similar to cast_to_variant.rs)
// so it would be great to figure out how to share this code
- _ => {
- // We shouldn't panic in production code, but this is a
- // placeholder until we implement more types
- // https://github.com/apache/arrow-rs/issues/8091
- debug_assert!(
- false,
- "Unsupported typed_value type: {}",
- typed_value.data_type()
- );
- Ok(Variant::Null)
- }
+ //
+ // Returning Variant::Null here would silently misreport stored values
+ // as null in release builds (https://github.com/apache/arrow-rs/issues/10597)
+ _ => Err(ArrowError::NotYetImplemented(format!(
+ "VariantArray does not yet support typed_value type: {}",
+ typed_value.data_type()
+ ))),
Review Comment:
Applied in cfbe39ae — your comment + error text verbatim (rustfmt reflowed the string continuation). GitHub wouldn't let me use the suggestion button on top of the pushed commit, so it went in by hand; credit to you for the wording.
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1152,17 +1152,13 @@ fn typed_value_to_variant(typed_value: &ArrayRef, index: usize) -> Result<Varian
}
// todo other types here (note this is very similar to cast_to_variant.rs)
// so it would be great to figure out how to share this code
- _ => {
- // We shouldn't panic in production code, but this is a
- // placeholder until we implement more types
- // https://github.com/apache/arrow-rs/issues/8091
- debug_assert!(
- false,
- "Unsupported typed_value type: {}",
- typed_value.data_type()
- );
- Ok(Variant::Null)
- }
+ //
+ // Returning Variant::Null here would silently misreport stored values
+ // as null in release builds (https://github.com/apache/arrow-rs/issues/10597)
Review Comment:
Removed in cfbe39ae.
--
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] Error instead of silent `Variant::Null` for unimplemented `typed_value` types [arrow-rs]
Posted by "raghav-reglobe (via GitHub)" <gi...@apache.org>.
raghav-reglobe commented on code in PR #10598:
URL: https://github.com/apache/arrow-rs/pull/10598#discussion_r3751040345
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1152,17 +1152,13 @@ fn typed_value_to_variant(typed_value: &ArrayRef, index: usize) -> Result<Varian
}
// todo other types here (note this is very similar to cast_to_variant.rs)
// so it would be great to figure out how to share this code
- _ => {
- // We shouldn't panic in production code, but this is a
- // placeholder until we implement more types
- // https://github.com/apache/arrow-rs/issues/8091
- debug_assert!(
- false,
- "Unsupported typed_value type: {}",
- typed_value.data_type()
- );
- Ok(Variant::Null)
- }
+ //
+ // Returning Variant::Null here would silently misreport stored values
+ // as null in release builds (https://github.com/apache/arrow-rs/issues/10597)
+ _ => Err(ArrowError::NotYetImplemented(format!(
Review Comment:
Yes — #10597 (linked in the description as "Closes #10597", and the new comment in this arm points there too). The #8091 link is the old code's — it goes away with the removed `debug_assert` block. #8091 was closed with the placeholder still in place, which is why #10597 was filed to track the remaining conversions (shredded objects/lists).
--
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] Error instead of silent `Variant::Null` for unimplemented `typed_value` types [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #10598:
URL: https://github.com/apache/arrow-rs/pull/10598#discussion_r3775690617
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1815,4 +1813,24 @@ mod test {
),]),
"Cast error: Cast failed at index 0 (array type: Decimal128(38, 10)): Invalid argument error: 123456789012345678901234567890123456789 is wider than max precision 38"
);
+ #[test]
+ fn try_value_errors_on_unimplemented_typed_value_type() {
+ use crate::{json_to_variant, shred_variant};
+ use arrow::array::StringArray;
+
+ let json: ArrayRef = Arc::new(StringArray::from(vec![r#"{"qty": 3}"#]));
+ let variant = json_to_variant(&json).unwrap();
+ let shred_type = DataType::Struct(vec![Field::new("qty", DataType::Int64, true)].into());
+ let shredded = shred_variant(&variant, &shred_type).unwrap();
+ // Object-shredded typed_value is not yet implemented: reading it must
+ // error, never silently return Variant::Null
+ // https://github.com/apache/arrow-rs/issues/10597
Review Comment:
```suggestion
// TODO: https://github.com/apache/arrow-rs/issues/10620
```
--
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] Error instead of silent `Variant::Null` for unimplemented `typed_value` types [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #10598:
URL: https://github.com/apache/arrow-rs/pull/10598#discussion_r3744093945
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1152,17 +1152,13 @@ fn typed_value_to_variant(typed_value: &ArrayRef, index: usize) -> Result<Varian
}
// todo other types here (note this is very similar to cast_to_variant.rs)
// so it would be great to figure out how to share this code
- _ => {
- // We shouldn't panic in production code, but this is a
- // placeholder until we implement more types
- // https://github.com/apache/arrow-rs/issues/8091
- debug_assert!(
- false,
- "Unsupported typed_value type: {}",
- typed_value.data_type()
- );
- Ok(Variant::Null)
- }
+ //
+ // Returning Variant::Null here would silently misreport stored values
+ // as null in release builds (https://github.com/apache/arrow-rs/issues/10597)
+ _ => Err(ArrowError::NotYetImplemented(format!(
Review Comment:
should this be a nyi error or some other error type? as in are there still data types we need to implement or we've already implemented all and this truly is an unsupported datatype?
cc @sdf-jkl @klion26
--
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] Error instead of silent `Variant::Null` for unimplemented `typed_value` types [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #10598:
URL: https://github.com/apache/arrow-rs/pull/10598
--
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] Error instead of silent `Variant::Null` for unimplemented `typed_value` types [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #10598:
URL: https://github.com/apache/arrow-rs/pull/10598#discussion_r3749564567
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1152,17 +1152,13 @@ fn typed_value_to_variant(typed_value: &ArrayRef, index: usize) -> Result<Varian
}
// todo other types here (note this is very similar to cast_to_variant.rs)
// so it would be great to figure out how to share this code
- _ => {
- // We shouldn't panic in production code, but this is a
- // placeholder until we implement more types
- // https://github.com/apache/arrow-rs/issues/8091
- debug_assert!(
- false,
- "Unsupported typed_value type: {}",
- typed_value.data_type()
- );
- Ok(Variant::Null)
- }
+ //
+ // Returning Variant::Null here would silently misreport stored values
+ // as null in release builds (https://github.com/apache/arrow-rs/issues/10597)
+ _ => Err(ArrowError::NotYetImplemented(format!(
Review Comment:
do we have an issue to link here? the comment above links to an issue already closed hence my confusion
- https://github.com/apache/arrow-rs/issues/8091
--
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] Error instead of silent `Variant::Null` for unimplemented `typed_value` types [arrow-rs]
Posted by "raghav-reglobe (via GitHub)" <gi...@apache.org>.
raghav-reglobe commented on code in PR #10598:
URL: https://github.com/apache/arrow-rs/pull/10598#discussion_r3775571913
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1152,17 +1152,13 @@ fn typed_value_to_variant(typed_value: &ArrayRef, index: usize) -> Result<Varian
}
// todo other types here (note this is very similar to cast_to_variant.rs)
// so it would be great to figure out how to share this code
- _ => {
- // We shouldn't panic in production code, but this is a
- // placeholder until we implement more types
- // https://github.com/apache/arrow-rs/issues/8091
- debug_assert!(
- false,
- "Unsupported typed_value type: {}",
- typed_value.data_type()
- );
- Ok(Variant::Null)
- }
+ //
+ // Returning Variant::Null here would silently misreport stored values
+ // as null in release builds (https://github.com/apache/arrow-rs/issues/10597)
+ _ => Err(ArrowError::NotYetImplemented(format!(
+ "VariantArray does not yet support typed_value type: {}",
+ typed_value.data_type()
+ ))),
Review Comment:
Applied in cfbe39ae — your comment + error text verbatim (rustfmt reflowed the string continuation).
--
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