[Variant] Support `['fieldName']` in VariantPath parser by klion26 · Pull Request #9276 · apache/arrow-rs

klion26 · 2026-01-27T06:51:42Z

Which issue does this PR close?

Closes [Variant] support .. and ['fieldName'] syntax in the VariantPath parser #9050.
close [Variant] Support escaping mechanism for VariantPath parsing #8954

Rationale for this change

What changes are included in this PR?

Add [fieldName] support in VariantPath parser, will throw an error if the parser fails.

Also support escaping \ inside brackets. If we force users to use brackets when the field contains special characters, maybe we can also close #8954

Sample behaviors(read more on the code doc)

[foo] -> filed foo
[2] -> index 2
[a.b] -> field a.b
[a\]b] -> field a]b
[a\xb] -> field axb

Are these changes tested?

Added tests

Are there any user-facing changes?

Yes, there are some user-facing changes, but parquet-variant is still experient for now, so maybe we don't need to wait for a major version.

alamb

The idea looks good to me -- thank you @klion26

I think other than the term unchecked this PR looks reasonable to me

cc @scovich in case you have other thoughts

alamb · 2026-01-27T22:52:13Z

parquet-variant/src/path.rs

+
+    /// Parses a path string, panics on invalid input.
+    /// Only use for tests for known-valid input.
+    pub fn from_str_unchecked(s: &'a str) -> Self {


In rust and other parts of this crate the "unchecked" word is often used to denote unsafe code -- specifically that could lead to undefined behavior

I suggest we don't call it "unchecked"

I think just from_str is probably fine ?

Thanks for the feedback. changed to from_str_or_panic. because clippy discourage to implement a from_str manually, and the std::str::FromStr::from_str returns Result<Self, Self::Err>; which is the same as TryFrom.

error: method `from_str` can be confused for the standard trait method `std::str::FromStr::from_str

How about just using try_from("foo").unwrap() ?

parquet-variant/src/path.rs

scovich · 2026-01-29T18:11:24Z

Also support escaping \ inside brackets. If we force users to use brackets when the field contains special characters, maybe we can also close #8954

Out of curiosity, why would we need to force escaping anything other than ]? Even a \. wouldn't be enough to make splitting on . safe, so we'd just need a simple parser that scans the string looking for matching ] after encountering a [?

klion26

@alamb @scovich thanks for the review and sorry for the late reply, I've been busy with other things lately.

Also support escaping \ inside brackets. If we force users to use brackets when the field contains special characters, maybe we can also close #8954

Out of curiosity, why would we need to force escaping anything other than ]? Even a \. wouldn't be enough to make splitting on . safe, so we'd just need a simple parser that scans the string looking for matching ] after encountering a [?

@scovich Sorry for the confusion. In the description, I want to say we only support special characters like '.' between '[' and ']', if the user wants to use special characters, he/she nees to use them between '[' and ']', so the path parser is simpler and easier to maintain. If we prefer to support special characters in addition to '[' and ']', we can continue with #8954 and then implement it.

klion26 · 2026-02-09T11:59:13Z

parquet-variant/src/path.rs

+
+    /// Parses a path string, panics on invalid input.
+    /// Only use for tests for known-valid input.
+    pub fn from_str_unchecked(s: &'a str) -> Self {


Thanks for the feedback. changed to from_str_or_panic. because clippy discourage to implement a from_str manually, and the std::str::FromStr::from_str returns Result<Self, Self::Err>; which is the same as TryFrom.

error: method `from_str` can be confused for the standard trait method `std::str::FromStr::from_str

parquet-variant/src/path.rs

alamb · 2026-02-11T12:11:25Z

I'll try and review this one over the next day or two to get it into the 58 release

Release arrow-rs / parquet Major version 58.0.0 (January 2026) #8466

alamb

Thanks @klion26 -- this is looking good. I am sorry for the delay in reviewing.

alamb · 2026-02-11T20:31:24Z

parquet-variant-compute/src/shred_variant.rs

 /// 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))


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

/// 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 /// ...

Fixed, this's indeed a better way.

alamb · 2026-02-11T20:32:09Z

parquet-variant-compute/src/variant_get.rs

        single_variant_get_test(
            r#"{"some_field": 1234}"#,
-            VariantPath::from("some_field"),
+            VariantPath::from_str_or_panic("some_field"),


I think this would be clearer to be honest:

Suggested change

VariantPath::from_str_or_panic("some_field"),

VariantPath::try_from("some_field").unwrap(),

Or maybe a helper like

fn parse_path(p: &str) -> VariantPath<'a> { VariantPath::try_from(p).unwrap(), }

And then

parse_path("some_field")

changed to try_from(..).unwrap()

parquet-variant/src/path.rs

alamb · 2026-02-11T20:34:12Z

parquet-variant/src/path.rs

    #[test]
    fn test_variant_path_empty_str() {
-        let path = VariantPath::from("");
+        let path = VariantPath::from_str_or_panic("");


I think

let path = VariantPath::try_from("").unwrap()

Is no less verbose and is easier to understand (as it uses the standard rust traits)

klion26

@alamb I've updated the code, please take another look, thanks.

klion26 · 2026-02-12T16:18:22Z

parquet-variant-compute/src/shred_variant.rs

-        let path: VariantPath<'a> = path.into();
+        let path: VariantPath<'a> = path
+            .try_into()
+            .map_err(|e| ArrowError::InvalidArgumentError(format!("{:?}", e)))?;


Use this line and L544, because we'll have VaraintPath -> VariantPath, which is Infallible, and there is no From<Infallible> for ArrowError, ArrowError is located in arrow-schema, not sure if it's appropriate to make the change there. If add From<Infallible> for ArrowError is better, I can change it.

alamb

Thanks @klion26 -- I think this change looks good to me

FYI @scovich

github-actions bot added the parquet-variant parquet-variant* crates label Jan 27, 2026

klion26 force-pushed the variant-path-field-in-bracket branch from 88e1964 to 3201b0e Compare January 27, 2026 08:50

alamb reviewed Jan 27, 2026

View reviewed changes

klion26 force-pushed the variant-path-field-in-bracket branch from d76d9e7 to 8e2dcdb Compare February 9, 2026 12:24

klion26 added 5 commits February 9, 2026 20:29

[Variant] Support field access in bracket for VariantPath

529ab76

fix VariantPath usage in tests

64f87c8

fix test

1357423

address comment

9426826

rebase main

634f236

klion26 force-pushed the variant-path-field-in-bracket branch from 8e2dcdb to 634f236 Compare February 9, 2026 12:38

klion26 commented Feb 9, 2026

View reviewed changes

alamb reviewed Feb 11, 2026

View reviewed changes

address comment

7a1d447

klion26 force-pushed the variant-path-field-in-bracket branch from 6e3c4ce to 7a1d447 Compare February 12, 2026 15:58

add comment back

bf43f7f

klion26 commented Feb 12, 2026

View reviewed changes

alamb added the api-change Changes to the arrow API label Feb 14, 2026

alamb approved these changes Feb 14, 2026

View reviewed changes

	VariantPath::from_str_or_panic("some_field"),
	VariantPath::try_from("some_field").unwrap(),

Conversation

klion26 commented Jan 27, 2026 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scovich commented Jan 29, 2026

Uh oh!

klion26 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb commented Feb 11, 2026

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klion26 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

klion26 commented Jan 27, 2026 •

edited by alamb

Loading