You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "foskey51 (via GitHub)" <gi...@apache.org> on 2025/12/17 18:22:50 UTC
[PR] feat: support array indices in VariantPath dot notation [arrow-rs]
foskey51 opened a new pull request, #9012:
URL: https://github.com/apache/arrow-rs/pull/9012
# Which issue does this PR close?
- Closes #8946
# What changes are included in this PR?
The PR adds support for parsing array index (eg. `foo.bar[3]`) with the help of parse_path fn. Currently the parser silently parses invalid segments as Field (eg., `foo[0`, `[0]`, `foo0]`, `foo[0][`)
#### Feedback requested
Whether to add stricter validation (throw an error) and reject the segment ? Or to keep the current behavior ?
# Are these changes tested?
yes, only for valid inputs
# Are there any user-facing changes?
no
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2635957454
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
Does the `flat_map` mean this will parse paths like `foo..bar` as `[foo]` `[bar`]?
I am not sure exactly what should happen when parsing a path like that 🤔
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
this is beautiful
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2636626718
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
A bit of quality time with an LLM produced the following (along with a bunch of unit tests to validate it works):
```rust
/// Parse a path string into a vector of [`VariantPathElement`].
///
/// Supports the following syntax:
/// - `""` - empty path
/// - `"foo"` - single field
/// - `"foo.bar"` - nested fields
/// - `"foo[1]"` - field with array index
/// - `"foo[1][2]"` - field with multiple array indices
/// - `"foo[1].bar"` - mixed field and array access
///
/// Field names can contain any characters except `.`, `[`, and `]`.
fn parse_path(s: &str) -> Result<Vec<VariantPathElement<'_>>, ArrowError> {
let scan_field = |start: usize| {
s[start..].find(['.', '[', ']']).map_or_else(|| s.len(), |p| start + p)
};
let mut elements = Vec::new();
let bytes = s.as_bytes();
let mut i = 0;
while i < bytes.len() {
match bytes[i] {
b'.' if elements.is_empty() => {
return Err(ArrowError::ParseError(format!(
"unexpected '.' at byte {i}"
)));
}
b'.' => {
// Skip the dot; a field must follow
i += 1;
let end = scan_field(i);
if end == i {
return Err(ArrowError::ParseError(match bytes.get(i) {
None => "path cannot end with '.'".into(),
Some(&c) => format!("unexpected '{}' at byte {i}", c as char),
}));
}
elements.push(VariantPathElement::field(&s[i..end]));
i = end;
}
b'[' => {
let (idx, end) = parse_index(s, i)?;
elements.push(VariantPathElement::index(idx));
i = end;
}
b']' => {
return Err(ArrowError::ParseError(format!(
"unexpected ']' at byte {i}"
)));
}
_ => {
let end = scan_field(i);
elements.push(VariantPathElement::field(&s[i..end]));
i = end;
}
}
}
Ok(elements)
}
/// Parse `[digits]` starting at `i` (which points to `[`).
/// Returns (index_value, position after `]`).
fn parse_index(s: &str, i: usize) -> Result<(usize, usize), ArrowError> {
let start = i + 1; // skip '['
// Find closing ']'
let end = match s[start..].find(']') {
Some(p) => start + p,
None => return Err(ArrowError::ParseError(format!("unterminated '[' at byte {i}"))),
};
// Parse the index
let idx_str = &s[start..end];
if idx_str.is_empty() || !idx_str.bytes().all(|b| b.is_ascii_digit()) {
return Err(ArrowError::ParseError(format!(
"empty or invalid index at byte {i}"
)));
}
let idx = idx_str
.parse::<usize>()
.map_err(|_| ArrowError::ParseError(format!("index too large at byte {start}")))?;
Ok((idx, end + 1))
}
/// Create from &str with support for dot notation and array indices.
///
/// # Example
/// ```
/// # use parquet_variant::VariantPath;
/// let path: VariantPath = "foo.bar[0]".try_into().unwrap();
/// ```
impl<'a> TryFrom<&'a str> for VariantPath<'a> {
type Error = ArrowError;
fn try_from(path: &'a str) -> Result<Self, Self::Error> {
parse_path(path).map(VariantPath::new)
}
}
```
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2636626718
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
A bit of quality time with an LLM produced the following (along with a bunch of unit tests to validate it works):
<details>
```rust
/// Parse a path string into a vector of [`VariantPathElement`].
///
/// Supports the following syntax:
/// - `""` - empty path
/// - `"foo"` - single field
/// - `"foo.bar"` - nested fields
/// - `"foo[1]"` - field with array index
/// - `"foo[1][2]"` - field with multiple array indices
/// - `"foo[1].bar"` - mixed field and array access
///
/// Field names can contain any characters except `.`, `[`, and `]`.
fn parse_path(s: &str) -> Result<Vec<VariantPathElement<'_>>, ArrowError> {
let scan_field = |start: usize| {
s[start..].find(['.', '[', ']']).map_or_else(|| s.len(), |p| start + p)
};
let mut elements = Vec::new();
let bytes = s.as_bytes();
let mut i = 0;
while i < bytes.len() {
match bytes[i] {
b'.' if elements.is_empty() => {
return Err(ArrowError::ParseError(format!(
"unexpected '.' at byte {i}"
)));
}
b'.' => {
// Skip the dot; a field must follow
i += 1;
let end = scan_field(i);
if end == i {
return Err(ArrowError::ParseError(match bytes.get(i) {
None => "path cannot end with '.'".into(),
Some(&c) => format!("unexpected '{}' at byte {i}", c as char),
}));
}
elements.push(VariantPathElement::field(&s[i..end]));
i = end;
}
b'[' => {
let (idx, end) = parse_index(s, i)?;
elements.push(VariantPathElement::index(idx));
i = end;
}
b']' => {
return Err(ArrowError::ParseError(format!(
"unexpected ']' at byte {i}"
)));
}
_ => {
let end = scan_field(i);
elements.push(VariantPathElement::field(&s[i..end]));
i = end;
}
}
}
Ok(elements)
}
/// Parse `[digits]` starting at `i` (which points to `[`).
/// Returns (index_value, position after `]`).
fn parse_index(s: &str, i: usize) -> Result<(usize, usize), ArrowError> {
let start = i + 1; // skip '['
// Find closing ']'
let end = match s[start..].find(']') {
Some(p) => start + p,
None => return Err(ArrowError::ParseError(format!("unterminated '[' at byte {i}"))),
};
// Parse the index
let idx_str = &s[start..end];
if idx_str.is_empty() || !idx_str.bytes().all(|b| b.is_ascii_digit()) {
return Err(ArrowError::ParseError(format!(
"empty or invalid index at byte {i}"
)));
}
let idx = idx_str
.parse::<usize>()
.map_err(|_| ArrowError::ParseError(format!("index too large at byte {start}")))?;
Ok((idx, end + 1))
}
/// Create from &str with support for dot notation and array indices.
///
/// # Example
/// ```
/// # use parquet_variant::VariantPath;
/// let path: VariantPath = "foo.bar[0]".try_into().unwrap();
/// ```
impl<'a> TryFrom<&'a str> for VariantPath<'a> {
type Error = ArrowError;
fn try_from(path: &'a str) -> Result<Self, Self::Error> {
parse_path(path).map(VariantPath::new)
}
}
```
</details>
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2636626718
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
A bit of quality time with an LLM produced the following (along with a bunch of unit tests to validate it works):
<details>
```rust
/// Parse a path string into a vector of [`VariantPathElement`].
///
/// Supports the following syntax:
/// - `""` - empty path
/// - `"foo"` - single field
/// - `"foo.bar"` - nested fields
/// - `"[1]"` - single array index
/// - `"[1][2]"` - multiple array indices
/// - `"foo[1]"` - field with array index
/// - `"foo[1][2]"` - field with multiple array indices
/// - `"foo[1].bar"` - mixed field and array access
/// - etc.
///
/// Field names can contain any characters except `.`, `[`, and `]`.
fn parse_path(s: &str) -> Result<Vec<VariantPathElement<'_>>, ArrowError> {
let scan_field = |start: usize| {
s[start..].find(['.', '[', ']']).map_or_else(|| s.len(), |p| start + p)
};
let mut elements = Vec::new();
let bytes = s.as_bytes();
let mut i = 0;
while i < bytes.len() {
match bytes[i] {
b'.' if elements.is_empty() => {
return Err(ArrowError::ParseError(format!(
"unexpected '.' at byte {i}"
)));
}
b'.' => {
// Skip the dot; a field must follow
i += 1;
let end = scan_field(i);
if end == i {
return Err(ArrowError::ParseError(match bytes.get(i) {
None => "path cannot end with '.'".into(),
Some(&c) => format!("unexpected '{}' at byte {i}", c as char),
}));
}
elements.push(VariantPathElement::field(&s[i..end]));
i = end;
}
b'[' => {
let (idx, end) = parse_index(s, i)?;
elements.push(VariantPathElement::index(idx));
i = end;
}
b']' => {
return Err(ArrowError::ParseError(format!(
"unexpected ']' at byte {i}"
)));
}
_ => {
let end = scan_field(i);
elements.push(VariantPathElement::field(&s[i..end]));
i = end;
}
}
}
Ok(elements)
}
/// Parse `[digits]` starting at `i` (which points to `[`).
/// Returns (index_value, position after `]`).
fn parse_index(s: &str, i: usize) -> Result<(usize, usize), ArrowError> {
let start = i + 1; // skip '['
// Find closing ']'
let end = match s[start..].find(']') {
Some(p) => start + p,
None => return Err(ArrowError::ParseError(format!("unterminated '[' at byte {i}"))),
};
// Parse the index
let idx_str = &s[start..end];
if idx_str.is_empty() || !idx_str.bytes().all(|b| b.is_ascii_digit()) {
return Err(ArrowError::ParseError(format!(
"empty or invalid index at byte {i}"
)));
}
let idx = idx_str
.parse::<usize>()
.map_err(|_| ArrowError::ParseError(format!("index too large at byte {start}")))?;
Ok((idx, end + 1))
}
/// Create from &str with support for dot notation and array indices.
///
/// # Example
/// ```
/// # use parquet_variant::VariantPath;
/// let path: VariantPath = "foo.bar[0]".try_into().unwrap();
/// ```
impl<'a> TryFrom<&'a str> for VariantPath<'a> {
type Error = ArrowError;
fn try_from(path: &'a str) -> Result<Self, Self::Error> {
parse_path(path).map(VariantPath::new)
}
}
```
</details>
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2636626718
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
A bit of quality time with an LLM produced the following (along with a bunch of unit tests to validate it works):
<details>
```rust
/// Parse a path string into a vector of [`VariantPathElement`].
///
/// Supports the following syntax:
/// - `""` - empty path
/// - `"foo"` - single field
/// - `"foo.bar"` - nested fields
/// - `"[1]"` - single array index
/// - `"[1][2]"` - multiple array indices
/// - `"foo[1]"` - field with array index
/// - `"foo[1][2]"` - field with multiple array indices
/// - `"foo[1].bar"` - mixed field and array access
///
/// Field names can contain any characters except `.`, `[`, and `]`.
fn parse_path(s: &str) -> Result<Vec<VariantPathElement<'_>>, ArrowError> {
let scan_field = |start: usize| {
s[start..].find(['.', '[', ']']).map_or_else(|| s.len(), |p| start + p)
};
let mut elements = Vec::new();
let bytes = s.as_bytes();
let mut i = 0;
while i < bytes.len() {
match bytes[i] {
b'.' if elements.is_empty() => {
return Err(ArrowError::ParseError(format!(
"unexpected '.' at byte {i}"
)));
}
b'.' => {
// Skip the dot; a field must follow
i += 1;
let end = scan_field(i);
if end == i {
return Err(ArrowError::ParseError(match bytes.get(i) {
None => "path cannot end with '.'".into(),
Some(&c) => format!("unexpected '{}' at byte {i}", c as char),
}));
}
elements.push(VariantPathElement::field(&s[i..end]));
i = end;
}
b'[' => {
let (idx, end) = parse_index(s, i)?;
elements.push(VariantPathElement::index(idx));
i = end;
}
b']' => {
return Err(ArrowError::ParseError(format!(
"unexpected ']' at byte {i}"
)));
}
_ => {
let end = scan_field(i);
elements.push(VariantPathElement::field(&s[i..end]));
i = end;
}
}
}
Ok(elements)
}
/// Parse `[digits]` starting at `i` (which points to `[`).
/// Returns (index_value, position after `]`).
fn parse_index(s: &str, i: usize) -> Result<(usize, usize), ArrowError> {
let start = i + 1; // skip '['
// Find closing ']'
let end = match s[start..].find(']') {
Some(p) => start + p,
None => return Err(ArrowError::ParseError(format!("unterminated '[' at byte {i}"))),
};
// Parse the index
let idx_str = &s[start..end];
if idx_str.is_empty() || !idx_str.bytes().all(|b| b.is_ascii_digit()) {
return Err(ArrowError::ParseError(format!(
"empty or invalid index at byte {i}"
)));
}
let idx = idx_str
.parse::<usize>()
.map_err(|_| ArrowError::ParseError(format!("index too large at byte {start}")))?;
Ok((idx, end + 1))
}
/// Create from &str with support for dot notation and array indices.
///
/// # Example
/// ```
/// # use parquet_variant::VariantPath;
/// let path: VariantPath = "foo.bar[0]".try_into().unwrap();
/// ```
impl<'a> TryFrom<&'a str> for VariantPath<'a> {
type Error = ArrowError;
fn try_from(path: &'a str) -> Result<Self, Self::Error> {
parse_path(path).map(VariantPath::new)
}
}
```
</details>
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2640273680
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
Interesting. It looks like the mini-parser I posted above is a strict subset of the databricks syntax linked above. Would it be a reasonable starting point to merge this PR and add the missing `['fieldName']` syntax as a follow-up? Or just expand the above suggestion into a full parser right away?
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2636626718
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
A bit of quality time with an LLM produced the following (along with a bunch of unit tests to validate it works):
<details>
```rust
/// Create from &str with support for dot notation and array indices.
///
/// # Example
/// ```
/// # use parquet_variant::VariantPath;
/// let path: VariantPath = "foo.bar[0]".try_into().unwrap();
/// ```
impl<'a> TryFrom<&'a str> for VariantPath<'a> {
type Error = ArrowError;
fn try_from(path: &'a str) -> Result<Self, Self::Error> {
parse_path(path).map(VariantPath::new)
}
}
/// Parse a path string into a vector of [`VariantPathElement`].
///
/// Supports the following syntax:
/// - `""` - empty path
/// - `"foo"` - single field
/// - `"foo.bar"` - nested fields
/// - `"[1]"` - single array index
/// - `"[1][2]"` - multiple array indices
/// - `"foo[1]"` - field with array index
/// - `"foo[1][2]"` - field with multiple array indices
/// - `"foo[1].bar"` - mixed field and array access
/// - etc.
///
/// Field names can contain any characters except `.`, `[`, and `]`.
fn parse_path(s: &str) -> Result<Vec<VariantPathElement<'_>>, ArrowError> {
let scan_field = |start: usize| {
s[start..].find(['.', '[', ']']).map_or_else(|| s.len(), |p| start + p)
};
let bytes = s.as_bytes();
if let Some(b'.') = bytes.first() {
return Err(ArrowError::ParseError("unexpected leading '.'".into()));
}
let mut elements = Vec::new();
let mut i = 0;
while i < bytes.len() {
let (elem, end) = match bytes[i] {
b'.' => {
i += 1; // skip the dot; a field must follow
let end = scan_field(i);
if end == i {
return Err(ArrowError::ParseError(match bytes.get(i) {
None => "unexpected trailing '.'".into(),
Some(&c) => format!("unexpected '{}' at byte {i}", c as char),
}));
}
(VariantPathElement::field(&s[i..end]), end)
}
b'[' => {
let (idx, end) = parse_index(s, i)?;
(VariantPathElement::index(idx), end)
}
b']' => {
return Err(ArrowError::ParseError(format!(
"unexpected ']' at byte {i}"
)));
}
_ => {
let end = scan_field(i);
(VariantPathElement::field(&s[i..end]), end)
}
};
elements.push(elem);
i = end;
}
Ok(elements)
}
/// Parse `[digits]` starting at `i` (which points to `[`).
/// Returns (index_value, position after `]`).
fn parse_index(s: &str, i: usize) -> Result<(usize, usize), ArrowError> {
let start = i + 1; // skip '['
// Find closing ']'
let end = match s[start..].find(']') {
Some(p) => start + p,
None => return Err(ArrowError::ParseError(format!("unterminated '[' at byte {i}"))),
};
let idx = s[start..end]
.parse()
.map_err(|_| ArrowError::ParseError(format!("invalid index at byte {start}")))?;
Ok((idx, end + 1))
}
```
</details>
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2649106117
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
- Filed https://github.com/apache/arrow-rs/issues/9050
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#issuecomment-3693932748
Thanks again @foskey51 @mhilton and @scovich
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2636626718
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
A bit of quality time with an LLM produced the following (along with a bunch of unit tests to validate it works):
<details>
```rust
/// Create from &str with support for dot notation and array indices.
///
/// # Example
/// ```
/// # use parquet_variant::VariantPath;
/// let path: VariantPath = "foo.bar[0]".try_into().unwrap();
/// ```
impl<'a> TryFrom<&'a str> for VariantPath<'a> {
type Error = ArrowError;
fn try_from(path: &'a str) -> Result<Self, Self::Error> {
parse_path(path).map(VariantPath::new)
}
}
/// Parse a path string into a vector of [`VariantPathElement`].
///
/// Supports the following syntax:
/// - `""` - empty path
/// - `"foo"` - single field
/// - `"foo.bar"` - nested fields
/// - `"[1]"` - single array index
/// - `"[1][2]"` - multiple array indices
/// - `"foo[1]"` - field with array index
/// - `"foo[1][2]"` - field with multiple array indices
/// - `"foo[1].bar"` - mixed field and array access
/// - etc.
///
/// Field names can contain any characters except `.`, `[`, and `]`.
fn parse_path(s: &str) -> Result<Vec<VariantPathElement<'_>>, ArrowError> {
let scan_field = |start: usize| {
s[start..].find(['.', '[', ']']).map_or_else(|| s.len(), |p| start + p)
};
let mut elements = Vec::new();
let bytes = s.as_bytes();
let mut i = 0;
while i < bytes.len() {
match bytes[i] {
b'.' if elements.is_empty() => {
return Err(ArrowError::ParseError(format!(
"unexpected '.' at byte {i}"
)));
}
b'.' => {
// Skip the dot; a field must follow
i += 1;
let end = scan_field(i);
if end == i {
return Err(ArrowError::ParseError(match bytes.get(i) {
None => "path cannot end with '.'".into(),
Some(&c) => format!("unexpected '{}' at byte {i}", c as char),
}));
}
elements.push(VariantPathElement::field(&s[i..end]));
i = end;
}
b'[' => {
let (idx, end) = parse_index(s, i)?;
elements.push(VariantPathElement::index(idx));
i = end;
}
b']' => {
return Err(ArrowError::ParseError(format!(
"unexpected ']' at byte {i}"
)));
}
_ => {
let end = scan_field(i);
elements.push(VariantPathElement::field(&s[i..end]));
i = end;
}
}
}
Ok(elements)
}
/// Parse `[digits]` starting at `i` (which points to `[`).
/// Returns (index_value, position after `]`).
fn parse_index(s: &str, i: usize) -> Result<(usize, usize), ArrowError> {
let start = i + 1; // skip '['
// Find closing ']'
let end = match s[start..].find(']') {
Some(p) => start + p,
None => return Err(ArrowError::ParseError(format!("unterminated '[' at byte {i}"))),
};
let idx = s[start..end]
.parse()
.map_err(|_| ArrowError::ParseError(format!("invalid index at byte {start}")))?;
Ok((idx, end + 1))
}
```
</details>
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2636626718
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
A bit of quality time with an LLM produced the following (along with a bunch of unit tests to validate it works):
<details>
```rust
/// Create from &str with support for dot notation and array indices.
///
/// # Example
/// ```
/// # use parquet_variant::VariantPath;
/// let path: VariantPath = "foo.bar[0]".try_into().unwrap();
/// ```
impl<'a> TryFrom<&'a str> for VariantPath<'a> {
type Error = ArrowError;
fn try_from(path: &'a str) -> Result<Self, Self::Error> {
parse_path(path).map(VariantPath::new)
}
}
/// Parse a path string into a vector of [`VariantPathElement`].
///
/// Supports the following syntax:
/// - `""` - empty path
/// - `"foo"` - single field
/// - `"foo.bar"` - nested fields
/// - `"[1]"` - single array index
/// - `"[1][2]"` - multiple array indices
/// - `"foo[1]"` - field with array index
/// - `"foo[1][2]"` - field with multiple array indices
/// - `"foo[1].bar"` - mixed field and array access
/// - etc.
///
/// Field names can contain any characters except `.`, `[`, and `]`.
fn parse_path(s: &str) -> Result<Vec<VariantPathElement<'_>>, ArrowError> {
let scan_field = |start: usize| {
s[start..].find(['.', '[', ']']).map_or_else(|| s.len(), |p| start + p)
};
let mut elements = Vec::new();
let bytes = s.as_bytes();
let mut i = 0;
while i < bytes.len() {
let (elem, end) = match bytes[i] {
b'.' if i == 0 => {
return Err(ArrowError::ParseError("unexpected '.' at byte 0".into()));
}
b'.' => {
i += 1; // skip the dot; a field must follow
let end = scan_field(i);
if end == i {
return Err(ArrowError::ParseError(match bytes.get(i) {
None => "path cannot end with '.'".into(),
Some(&c) => format!("unexpected '{}' at byte {i}", c as char),
}));
}
(VariantPathElement::field(&s[i..end]), end)
}
b'[' => {
let (idx, end) = parse_index(s, i)?;
(VariantPathElement::index(idx), end)
}
b']' => {
return Err(ArrowError::ParseError(format!(
"unexpected ']' at byte {i}"
)));
}
_ => {
let end = scan_field(i);
(VariantPathElement::field(&s[i..end]), end)
}
};
elements.push(elem);
i = end;
}
Ok(elements)
}
/// Parse `[digits]` starting at `i` (which points to `[`).
/// Returns (index_value, position after `]`).
fn parse_index(s: &str, i: usize) -> Result<(usize, usize), ArrowError> {
let start = i + 1; // skip '['
// Find closing ']'
let end = match s[start..].find(']') {
Some(p) => start + p,
None => return Err(ArrowError::ParseError(format!("unterminated '[' at byte {i}"))),
};
let idx = s[start..end]
.parse()
.map_err(|_| ArrowError::ParseError(format!("invalid index at byte {start}")))?;
Ok((idx, end + 1))
}
```
</details>
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2636415342
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
Now that you mention, it seems like the existing code already had that bug?
In theory, that should be a malformed path that causes an error. Ditto for mismatched `[` or `]`, or e.g. `[a.b]` (which this PR would incorrectly attempt to treat as `[a` and `b]`).
It seems like we'll need a more sophisticated (and integrated) parsing approach here?
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "mhilton (via GitHub)" <gi...@apache.org>.
mhilton commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2630070655
##########
parquet-variant/src/path.rs:
##########
@@ -223,4 +221,17 @@ mod tests {
let path = VariantPath::from_iter([p]);
assert!(!path.is_empty());
}
+
+ #[test]
+ fn test_variant_path_dot_notation_with_array_index() {
+ let path = VariantPath::from("city.store.books[3].title");
Review Comment:
It'd be good to add a test case where the path is just an index `[3]` and one where it starts with an index `[3].title`.
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2637466197
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
Meanwhile, even the parser I cooked up above is technically incomplete, because JSON allows field names to contain any character (including `.`, `[`] and `]` which my suggestion would reject as ambiguous). The only way to avoid that is with some kind of escaping syntax. I believe Iceberg uses canonical JSONpath, for example.
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2636626718
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
A bit of quality time with an LLM produced the following (along with a bunch of unit tests to validate it works):
<details>
```rust
/// Parse a path string into a vector of [`VariantPathElement`].
///
/// Supports the following syntax:
/// - `""` - empty path
/// - `"foo"` - single field
/// - `"foo.bar"` - nested fields
/// - `"[1]"` - single array index
/// - `"[1][2]"` - multiple array indices
/// - `"foo[1]"` - field with array index
/// - `"foo[1][2]"` - field with multiple array indices
/// - `"foo[1].bar"` - mixed field and array access
/// - etc.
///
/// Field names can contain any characters except `.`, `[`, and `]`.
fn parse_path(s: &str) -> Result<Vec<VariantPathElement<'_>>, ArrowError> {
let scan_field = |start: usize| {
s[start..].find(['.', '[', ']']).map_or_else(|| s.len(), |p| start + p)
};
let mut elements = Vec::new();
let bytes = s.as_bytes();
let mut i = 0;
while i < bytes.len() {
match bytes[i] {
b'.' if elements.is_empty() => {
return Err(ArrowError::ParseError(format!(
"unexpected '.' at byte {i}"
)));
}
b'.' => {
// Skip the dot; a field must follow
i += 1;
let end = scan_field(i);
if end == i {
return Err(ArrowError::ParseError(match bytes.get(i) {
None => "path cannot end with '.'".into(),
Some(&c) => format!("unexpected '{}' at byte {i}", c as char),
}));
}
elements.push(VariantPathElement::field(&s[i..end]));
i = end;
}
b'[' => {
let (idx, end) = parse_index(s, i)?;
elements.push(VariantPathElement::index(idx));
i = end;
}
b']' => {
return Err(ArrowError::ParseError(format!(
"unexpected ']' at byte {i}"
)));
}
_ => {
let end = scan_field(i);
elements.push(VariantPathElement::field(&s[i..end]));
i = end;
}
}
}
Ok(elements)
}
/// Parse `[digits]` starting at `i` (which points to `[`).
/// Returns (index_value, position after `]`).
fn parse_index(s: &str, i: usize) -> Result<(usize, usize), ArrowError> {
let start = i + 1; // skip '['
// Find closing ']'
let end = match s[start..].find(']') {
Some(p) => start + p,
None => return Err(ArrowError::ParseError(format!("unterminated '[' at byte {i}"))),
};
let idx = s[start..end]
.parse()
.map_err(|_| ArrowError::ParseError(format!("invalid index at byte {start}")))?;
Ok((idx, end + 1))
}
/// Create from &str with support for dot notation and array indices.
///
/// # Example
/// ```
/// # use parquet_variant::VariantPath;
/// let path: VariantPath = "foo.bar[0]".try_into().unwrap();
/// ```
impl<'a> TryFrom<&'a str> for VariantPath<'a> {
type Error = ArrowError;
fn try_from(path: &'a str) -> Result<Self, Self::Error> {
parse_path(path).map(VariantPath::new)
}
}
```
</details>
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "foskey51 (via GitHub)" <gi...@apache.org>.
foskey51 commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2637475021
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
There's an open issue regarding the escaping syntax https://github.com/apache/arrow-rs/issues/8954
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2636626718
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
A bit of quality time with an LLM produced the following (along with a bunch of unit tests to validate it works):
<details>
```rust
/// Create from &str with support for dot notation and array indices.
///
/// # Example
/// ```
/// # use parquet_variant::VariantPath;
/// let path: VariantPath = "foo.bar[0]".try_into().unwrap();
/// ```
impl<'a> TryFrom<&'a str> for VariantPath<'a> {
type Error = ArrowError;
fn try_from(path: &'a str) -> Result<Self, Self::Error> {
parse_path(path).map(VariantPath::new)
}
}
/// Parse a path string into a vector of [`VariantPathElement`].
///
/// Supports the following syntax:
/// - `""` - empty path
/// - `"foo"` - single field
/// - `"foo.bar"` - nested fields
/// - `"[1]"` - single array index
/// - `"[1][2]"` - multiple array indices
/// - `"foo[1]"` - field with array index
/// - `"foo[1][2]"` - field with multiple array indices
/// - `"foo[1].bar"` - mixed field and array access
/// - etc.
///
/// Field names can contain any characters except `.`, `[`, and `]`.
fn parse_path(s: &str) -> Result<Vec<VariantPathElement<'_>>, ArrowError> {
let scan_field = |start: usize| {
s[start..].find(['.', '[', ']']).map_or_else(|| s.len(), |p| start + p)
};
let mut elements = Vec::new();
let bytes = s.as_bytes();
let mut i = 0;
while i < bytes.len() {
let (elem, end) = match bytes[i] {
b'.' if i == 0 => {
return Err(ArrowError::ParseError("unexpected leading '.'".into()));
}
b'.' => {
i += 1; // skip the dot; a field must follow
let end = scan_field(i);
if end == i {
return Err(ArrowError::ParseError(match bytes.get(i) {
None => "path cannot end with '.'".into(),
Some(&c) => format!("unexpected '{}' at byte {i}", c as char),
}));
}
(VariantPathElement::field(&s[i..end]), end)
}
b'[' => {
let (idx, end) = parse_index(s, i)?;
(VariantPathElement::index(idx), end)
}
b']' => {
return Err(ArrowError::ParseError(format!(
"unexpected ']' at byte {i}"
)));
}
_ => {
let end = scan_field(i);
(VariantPathElement::field(&s[i..end]), end)
}
};
elements.push(elem);
i = end;
}
Ok(elements)
}
/// Parse `[digits]` starting at `i` (which points to `[`).
/// Returns (index_value, position after `]`).
fn parse_index(s: &str, i: usize) -> Result<(usize, usize), ArrowError> {
let start = i + 1; // skip '['
// Find closing ']'
let end = match s[start..].find(']') {
Some(p) => start + p,
None => return Err(ArrowError::ParseError(format!("unterminated '[' at byte {i}"))),
};
let idx = s[start..end]
.parse()
.map_err(|_| ArrowError::ParseError(format!("invalid index at byte {start}")))?;
Ok((idx, end + 1))
}
```
</details>
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2636626718
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
A bit of quality time with an LLM produced the following (along with a bunch of unit tests to validate it works):
<details>
```rust
/// Create from &str with support for dot notation and array indices.
///
/// # Example
/// ```
/// # use parquet_variant::VariantPath;
/// let path: VariantPath = "foo.bar[0]".try_into().unwrap();
/// ```
impl<'a> TryFrom<&'a str> for VariantPath<'a> {
type Error = ArrowError;
fn try_from(path: &'a str) -> Result<Self, Self::Error> {
parse_path(path).map(VariantPath::new)
}
}
/// Parse a path string into a vector of [`VariantPathElement`].
///
/// Supports the following syntax:
/// - `""` - empty path
/// - `"foo"` - single field
/// - `"foo.bar"` - nested fields
/// - `"[1]"` - single array index
/// - `"[1][2]"` - multiple array indices
/// - `"foo[1]"` - field with array index
/// - `"foo[1][2]"` - field with multiple array indices
/// - `"foo[1].bar"` - mixed field and array access
/// - etc.
///
/// Field names can contain any characters except `.`, `[`, and `]`.
fn parse_path(s: &str) -> Result<Vec<VariantPathElement<'_>>, ArrowError> {
let scan_field = |start: usize| {
s[start..].find(['.', '[', ']']).map_or_else(|| s.len(), |p| start + p)
};
let mut elements = Vec::new();
let bytes = s.as_bytes();
let mut i = 0;
while i < bytes.len() {
match bytes[i] {
b'.' if i == 0 => {
return Err(ArrowError::ParseError("unexpected '.' at byte 0".into()));
}
b'.' => {
// Skip the dot; a field must follow
i += 1;
let end = scan_field(i);
if end == i {
return Err(ArrowError::ParseError(match bytes.get(i) {
None => "path cannot end with '.'".into(),
Some(&c) => format!("unexpected '{}' at byte {i}", c as char),
}));
}
elements.push(VariantPathElement::field(&s[i..end]));
i = end;
}
b'[' => {
let (idx, end) = parse_index(s, i)?;
elements.push(VariantPathElement::index(idx));
i = end;
}
b']' => {
return Err(ArrowError::ParseError(format!(
"unexpected ']' at byte {i}"
)));
}
_ => {
let end = scan_field(i);
elements.push(VariantPathElement::field(&s[i..end]));
i = end;
}
}
}
Ok(elements)
}
/// Parse `[digits]` starting at `i` (which points to `[`).
/// Returns (index_value, position after `]`).
fn parse_index(s: &str, i: usize) -> Result<(usize, usize), ArrowError> {
let start = i + 1; // skip '['
// Find closing ']'
let end = match s[start..].find(']') {
Some(p) => start + p,
None => return Err(ArrowError::ParseError(format!("unterminated '[' at byte {i}"))),
};
let idx = s[start..end]
.parse()
.map_err(|_| ArrowError::ParseError(format!("invalid index at byte {start}")))?;
Ok((idx, end + 1))
}
```
</details>
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2637466197
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
Meanwhile, even the parser I cooked up above is technically incomplete, because JSON allows field names to contain any character (including `.`, `[` and `]` which my suggestion would reject as ambiguous). The only way to avoid that is with some kind of escaping syntax. I believe Iceberg uses canonical JSONpath, for example.
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2636626718
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
A bit of quality time with an LLM produced the following (along with a bunch of unit tests to validate it works):
<details>
```rust
/// Create from &str with support for dot notation and array indices.
///
/// # Example
/// ```
/// # use parquet_variant::VariantPath;
/// let path: VariantPath = "foo.bar[0]".try_into().unwrap();
/// ```
impl<'a> TryFrom<&'a str> for VariantPath<'a> {
type Error = ArrowError;
fn try_from(path: &'a str) -> Result<Self, Self::Error> {
parse_path(path).map(VariantPath::new)
}
}
/// Parse a path string into a vector of [`VariantPathElement`].
///
/// Supports the following syntax:
/// - `""` - empty path
/// - `"foo"` - single field
/// - `"foo.bar"` - nested fields
/// - `"[1]"` - single array index
/// - `"[1][2]"` - multiple array indices
/// - `"foo[1]"` - field with array index
/// - `"foo[1][2]"` - field with multiple array indices
/// - `"foo[1].bar"` - mixed field and array access
/// - etc.
///
/// Field names can contain any characters except `.`, `[`, and `]`.
fn parse_path(s: &str) -> Result<Vec<VariantPathElement<'_>>, ArrowError> {
let scan_field = |start: usize| {
s[start..].find(['.', '[', ']']).map_or_else(|| s.len(), |p| start + p)
};
let mut elements = Vec::new();
let bytes = s.as_bytes();
let mut i = 0;
while i < bytes.len() {
match bytes[i] {
b'.' if i == 0 => {
return Err(ArrowError::ParseError("unexpected leading '.'".into()));
}
b'.' => {
// Skip the dot; a field must follow
i += 1;
let end = scan_field(i);
if end == i {
return Err(ArrowError::ParseError(match bytes.get(i) {
None => "path cannot end with '.'".into(),
Some(&c) => format!("unexpected '{}' at byte {i}", c as char),
}));
}
elements.push(VariantPathElement::field(&s[i..end]));
i = end;
}
b'[' => {
let (idx, end) = parse_index(s, i)?;
elements.push(VariantPathElement::index(idx));
i = end;
}
b']' => {
return Err(ArrowError::ParseError(format!(
"unexpected ']' at byte {i}"
)));
}
_ => {
let end = scan_field(i);
elements.push(VariantPathElement::field(&s[i..end]));
i = end;
}
}
}
Ok(elements)
}
/// Parse `[digits]` starting at `i` (which points to `[`).
/// Returns (index_value, position after `]`).
fn parse_index(s: &str, i: usize) -> Result<(usize, usize), ArrowError> {
let start = i + 1; // skip '['
// Find closing ']'
let end = match s[start..].find(']') {
Some(p) => start + p,
None => return Err(ArrowError::ParseError(format!("unterminated '[' at byte {i}"))),
};
let idx = s[start..end]
.parse()
.map_err(|_| ArrowError::ParseError(format!("invalid index at byte {start}")))?;
Ok((idx, end + 1))
}
```
</details>
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2636626718
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
A bit of quality time with an LLM produced the following (along with a bunch of unit tests to validate it works):
<details>
```rust
/// Create from &str with support for dot notation and array indices.
///
/// # Example
/// ```
/// # use parquet_variant::VariantPath;
/// let path: VariantPath = "foo.bar[0]".try_into().unwrap();
/// ```
impl<'a> TryFrom<&'a str> for VariantPath<'a> {
type Error = ArrowError;
fn try_from(path: &'a str) -> Result<Self, Self::Error> {
parse_path(path).map(VariantPath::new)
}
}
/// Parse a path string into a vector of [`VariantPathElement`].
///
/// Supports the following syntax:
/// - `""` - empty path
/// - `"foo"` - single field
/// - `"foo.bar"` - nested fields
/// - `"[1]"` - single array index
/// - `"[1][2]"` - multiple array indices
/// - `"foo[1]"` - field with array index
/// - `"foo[1][2]"` - field with multiple array indices
/// - `"foo[1].bar"` - mixed field and array access
/// - etc.
///
/// Field names can contain any characters except `.`, `[`, and `]`.
fn parse_path(s: &str) -> Result<Vec<VariantPathElement<'_>>, ArrowError> {
let scan_field = |start: usize| {
s[start..].find(['.', '[', ']']).map_or_else(|| s.len(), |p| start + p)
};
let mut elements = Vec::new();
let bytes = s.as_bytes();
let mut i = 0;
while i < bytes.len() {
let (elem, end) = match bytes[i] {
b'.' if i == 0 => {
return Err(ArrowError::ParseError("unexpected leading '.'".into()));
}
b'.' => {
i += 1; // skip the dot; a field must follow
let end = scan_field(i);
if end == i {
return Err(ArrowError::ParseError(match bytes.get(i) {
None => "unexpected trailing '.'".into(),
Some(&c) => format!("unexpected '{}' at byte {i}", c as char),
}));
}
(VariantPathElement::field(&s[i..end]), end)
}
b'[' => {
let (idx, end) = parse_index(s, i)?;
(VariantPathElement::index(idx), end)
}
b']' => {
return Err(ArrowError::ParseError(format!(
"unexpected ']' at byte {i}"
)));
}
_ => {
let end = scan_field(i);
(VariantPathElement::field(&s[i..end]), end)
}
};
elements.push(elem);
i = end;
}
Ok(elements)
}
/// Parse `[digits]` starting at `i` (which points to `[`).
/// Returns (index_value, position after `]`).
fn parse_index(s: &str, i: usize) -> Result<(usize, usize), ArrowError> {
let start = i + 1; // skip '['
// Find closing ']'
let end = match s[start..].find(']') {
Some(p) => start + p,
None => return Err(ArrowError::ParseError(format!("unterminated '[' at byte {i}"))),
};
let idx = s[start..end]
.parse()
.map_err(|_| ArrowError::ParseError(format!("invalid index at byte {start}")))?;
Ok((idx, end + 1))
}
```
</details>
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "foskey51 (via GitHub)" <gi...@apache.org>.
foskey51 commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2637085626
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
I initially considered using `TryFrom` with explicit error boundaries , but doing so would require changing an existing public API, which would be a breaking change
https://github.com/apache/arrow-rs/blob/240cbf4f838387445b0209db4b14dbb277b05a12/parquet-variant-compute/src/shred_variant.rs#L709-L717
and as @scovich mentioned the current approach can also parse invalid inputs as fields, making debugging hard. So I'm a bit skeptical about how to proceed from here.
--
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] feat: support array indices in VariantPath dot notation [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9012:
URL: https://github.com/apache/arrow-rs/pull/9012#discussion_r2637464925
##########
parquet-variant/src/path.rs:
##########
@@ -112,11 +114,7 @@ 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> {
fn from(path: &'a str) -> Self {
- if path.is_empty() {
- VariantPath::new(vec![])
- } else {
- VariantPath::new(path.split('.').map(Into::into).collect())
- }
+ VariantPath::new(path.split(".").flat_map(parse_path).collect())
Review Comment:
AFAIK, variant is still experimental, so breaking changes are ok?
--
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