alamb commented on code in PR #9276:
URL: https://github.com/apache/arrow-rs/pull/9276#discussion_r2795377987
##########
parquet-variant-compute/src/shred_variant.rs:
##########
@@ -496,12 +496,12 @@ impl IntoShreddingField for (DataType, bool) {
/// // Define the shredding schema using the builder
/// let shredding_type = ShreddedSchemaBuilder::default()
/// // store the "time" field as a separate UTC timestamp
-/// .with_path("time", (&DataType::Timestamp(TimeUnit::Nanosecond,
Some("UTC".into())), true))
+/// .with_path(VariantPath::from_str_or_panic("time"),
(&DataType::Timestamp(TimeUnit::Nanosecond, Some("UTC".into())), true))
Review Comment:
This looks so ugly compared to the previous version
It seems if we are going to do this, we should probably also change the
signature of `with_path` because now ` P: Into<VariantPath<'a>>,` is
unlikely to match anything other than VariantPath
What if we changed the signature to take a `TryFrom` and return the correct
error?
Then the example could look like
```rust
/// let shredding_type = ShreddedSchemaBuilder::default()
/// // store the "time" field as a separate UTC timestamp
/// .with_path("time", &DataType::Timestamp(TimeUnit::Nanosecond,
Some("UTC".into())), true))? <-- note the ? here
/// ...
```
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -390,7 +390,7 @@ mod test {
fn get_primitive_variant_field() {
single_variant_get_test(
r#"{"some_field": 1234}"#,
- VariantPath::from("some_field"),
+ VariantPath::from_str_or_panic("some_field"),
Review Comment:
I think this would be clearer to be honest:
```suggestion
VariantPath::try_from("some_field").unwrap(),
```
Or maybe a helper like
```rust
fn parse_path(p: &str) -> VariantPath<'a> {
VariantPath::try_from(p).unwrap(),
}
```
And then
```rust
parse_path("some_field")
```
##########
parquet-variant/src/path.rs:
##########
@@ -112,9 +128,11 @@ impl<'a> From<Vec<VariantPathElement<'a>>> for
VariantPath<'a> {
}
/// Create from &str with support for dot notation
-impl<'a> From<&'a str> for VariantPath<'a> {
Review Comment:
Yes I agree using a TryFrom makes more sense
##########
parquet-variant/src/path.rs:
##########
@@ -211,7 +229,7 @@ mod tests {
#[test]
fn test_variant_path_empty_str() {
- let path = VariantPath::from("");
+ let path = VariantPath::from_str_or_panic("");
Review Comment:
I think
```rust
let path = VariantPath::try_from("").unwrap()
```
Is no less verbose and is easier to understand (as it uses the standard rust
traits)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
