This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-11232-0b94195639ccbae0d615596cdb11445cde1441b6 in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
commit dc59495122884e4aced624e6744f268131472094 Author: Bryan Dang <[email protected]> AuthorDate: Tue Oct 27 13:58:02 2026 +0000 perf(variant): avoid bracket path string allocations (#11232) # Which issue does this PR close? - Closes #9493. # Rationale for this change `parse_in_bracket` currently allocates a `String` for every bracket expression, including indexes, wildcards, and quoted fields that require no unescaping. `VariantPathElement::Field` already stores `Cow<str>`, so unescaped field names can borrow directly from the input path. # What changes are included in this PR? - Defer creation of the unescaped `String` until the first backslash is encountered. - Borrow the original bracket-content slice when no escape processing is required. - Preserve ownership for escaped fields and strip their surrounding quotes in place. - Add tests proving unescaped bracket fields use `Cow::Borrowed` while escaped fields use `Cow::Owned`. This branch includes `main` with #11231 (closing brackets inside quoted fields). The quote tracking added there relied on the eagerly built buffer, so it is adapted here: an opening quote is now detected by its offset in the bracket content rather than by the buffer being empty, and quoted fields containing `]` (e.g. `['a]b']`) still borrow. # Are these changes tested? Yes. The existing path parsing tests cover behavior, and a new test asserts the ownership of each case: empty, plain, and `]`-containing quoted fields are borrowed, and an escaped field is owned. The benchmark for this change is in #11508 (`cargo bench -p parquet-variant --bench variant_path`), split out as requested so it is available on `main` as a baseline. I will merge `main` here once it lands so the numbers can be reproduced. Earlier local measurements, taken with a temporary Criterion harness on aarch64 macOS before #11231 was merged in: | Case | Before | After | Change | |---|---:|---:|---:| | Three quoted bracket fields | 300.22 ns | 54.10 ns | -81.7% | | Three bracket indexes | 146.13 ns | 45.57 ns | -68.2% | | Escaped bracket field | 140.41 ns | 111.37 ns | -24.9% | | Dot-path control | 38.68 ns | 40.88 ns | Within noise | # Are there any user-facing changes? No semantic or public API changes. Parsed paths remain the same; unescaped bracket fields now borrow their names instead of allocating owned strings. # AI usage OpenAI Codex was used to inspect the parser, help implement the lazy allocation path, construct ownership tests, and run validation and benchmarks. Claude Code was used to adapt the change to #11231 after merging `main`. I reviewed the resulting code and behavior and can own and debug the change. --------- Co-authored-by: Claude Opus 5.5 <[email protected]> Co-authored-by: Kosta Tarasov <[email protected]> --- parquet-variant/src/path.rs | 17 +++++++++++++ parquet-variant/src/utils.rs | 57 +++++++++++++++++++++++++++++++------------- 2 files changed, 57 insertions(+), 17 deletions(-) diff --git a/parquet-variant/src/path.rs b/parquet-variant/src/path.rs index 98a6e916d8..b42e5eae49 100644 --- a/parquet-variant/src/path.rs +++ b/parquet-variant/src/path.rs @@ -349,6 +349,23 @@ mod tests { assert_eq!(path, expected); } + #[test] + fn test_variant_path_bracket_field_ownership() { + use VariantPathElement::Field; + + let path = VariantPath::try_from(r#"[""]["plain"]["escaped\]field"]['a]b']"#).unwrap(); + + assert!(matches!( + &path[..], + [ + Field { name: Cow::Borrowed("") }, + Field { name: Cow::Borrowed("plain") }, + Field { name: Cow::Owned(escaped) }, + Field { name: Cow::Borrowed("a]b") }, + ] if escaped == "escaped]field" + )); + } + #[test] fn test_invalid_path_parse() { // Leading dot diff --git a/parquet-variant/src/utils.rs b/parquet-variant/src/utils.rs index 049ab82c99..d5610cdb26 100644 --- a/parquet-variant/src/utils.rs +++ b/parquet-variant/src/utils.rs @@ -14,7 +14,7 @@ // KIND, either express or implied. See the License for the // specific language governing permissions and limitations // under the License. -use std::{array::TryFromSliceError, ops::Range, str}; +use std::{array::TryFromSliceError, borrow::Cow, ops::Range, str}; use crate::VariantPathElement; use arrow_schema::ArrowError; @@ -234,7 +234,7 @@ pub(crate) fn parse_path(s: &str) -> Result<Vec<VariantPathElement<'_>>, ArrowEr fn parse_in_bracket(s: &str, i: usize) -> Result<(VariantPathElement<'_>, usize), ArrowError> { let start = i + 1; // skip '[' - let mut unescaped = String::new(); + let mut unescaped = None; let mut chars = s[start..].char_indices(); let mut end = None; let mut quote = None; @@ -243,6 +243,8 @@ fn parse_in_bracket(s: &str, i: usize) -> Result<(VariantPathElement<'_>, usize) match c { // Escape: take next char literally '\\' => { + let unescaped = + unescaped.get_or_insert_with(|| String::from(&s[start..start + offset])); if let Some((_, next)) = chars.next() { unescaped.push(next); } @@ -250,11 +252,14 @@ fn parse_in_bracket(s: &str, i: usize) -> Result<(VariantPathElement<'_>, usize) } '\'' | '"' if quote == Some(c) => { quote = None; - unescaped.push(c); + if let Some(unescaped) = &mut unescaped { + unescaped.push(c); + } } - '\'' | '"' if quote.is_none() && unescaped.is_empty() => { + // Every consumed char advances the offset, so a quote can only + // open the bracket content at offset 0 + '\'' | '"' if quote.is_none() && offset == 0 => { quote = Some(c); - unescaped.push(c); } ']' if quote.is_none() => { // An unescaped ']' outside quotes ends the bracket @@ -262,7 +267,9 @@ fn parse_in_bracket(s: &str, i: usize) -> Result<(VariantPathElement<'_>, usize) break; } _ => { - unescaped.push(c); + if let Some(unescaped) = &mut unescaped { + unescaped.push(c); + } } } } @@ -271,23 +278,39 @@ fn parse_in_bracket(s: &str, i: usize) -> Result<(VariantPathElement<'_>, usize) return Err(ArrowError::ParseError(format!("Unclosed '[' at byte {i}"))); }; - let element = if let Some(inner) = unescaped - .strip_prefix('\'') - .and_then(|s| s.strip_suffix('\'')) - .or_else(|| unescaped.strip_prefix('"')?.strip_suffix('"')) - { + let value = match unescaped { + Some(value) => Cow::Owned(value), + None => Cow::Borrowed(&s[start..end]), + }; + let element = parse_bracket_element(value)?; + + Ok((element, end + 1)) +} + +fn parse_bracket_element(value: Cow<'_, str>) -> Result<VariantPathElement<'_>, ArrowError> { + let quoted = matches!(value.as_bytes(), [b'\'', .., b'\''] | [b'"', .., b'"']); + + if quoted { // Quoted field name, e.g., ['field'] or ['123'] or ["123"] - VariantPathElement::field(inner.to_string()) - } else if unescaped == "*" { + return Ok(VariantPathElement::field(match value { + Cow::Borrowed(value) => Cow::Borrowed(&value[1..value.len() - 1]), + Cow::Owned(mut value) => { + value.pop(); + value.remove(0); + Cow::Owned(value) + } + })); + } + + let element = if value == "*" { VariantPathElement::list_element() } else { - let Ok(idx) = unescaped.parse() else { + let Ok(idx) = value.parse() else { return Err(ArrowError::ParseError(format!( - "Invalid token in bracket request: `{unescaped}`. Expected `*`, a quoted string, or a number(e.g., `[*]`, `['field']`, or `[123]`)" + "Invalid token in bracket request: `{value}`. Expected `*`, a quoted string, or a number(e.g., `[*]`, `['field']`, or `[123]`)" ))); }; VariantPathElement::index(idx) }; - - Ok((element, end + 1)) + Ok(element) }
