You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "cakeni (via GitHub)" <gi...@apache.org> on 2026/08/11 13:26:50 UTC
[PR] [Variant] Support list paths in ShreddedSchemaBuilder [arrow-rs]
cakeni opened a new pull request, #10635:
URL: https://github.com/apache/arrow-rs/pull/10635
# Which issue does this PR close?
- Closes #10615.
# Rationale for this change
`ShreddedSchemaBuilder` can already parse indexed Variant paths, and core Variant shredding supports lists, but schema construction currently panics when a path reaches an index. This prevents callers from describing list element schemas through the builder.
# What changes are included in this PR?
- Add a list node to the builder's intermediate schema tree.
- Treat numeric indexes as references to the shared list element schema, including for nested lists.
- Document the index behavior and add coverage for list-of-struct and nested-list schemas.
- Build the existing list shredding test schema through `ShreddedSchemaBuilder`.
# Are these changes tested?
Yes. `cargo test -p parquet-variant-compute test_variant_schema_builder` passes (11 tests).
# Are there any user-facing changes?
Yes. Callers can now use indexed paths such as `items[0].id` when constructing shredding schemas. This is backward compatible.
--
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] [Variant] Support list paths in ShreddedSchemaBuilder [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl merged PR #10635:
URL: https://github.com/apache/arrow-rs/pull/10635
--
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] [Variant] Support list paths in ShreddedSchemaBuilder [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #10635:
URL: https://github.com/apache/arrow-rs/pull/10635#discussion_r3770497837
##########
parquet-variant-compute/src/shred_variant.rs:
##########
@@ -2898,6 +2911,49 @@ mod tests {
Ok(())
}
+ #[test]
+ fn test_variant_schema_builder_list() -> Result<()> {
+ let shredding_type = ShreddedSchemaBuilder::default()
+ .with_path("items[0].id", &DataType::Int64)?
+ .with_path("items[42].name", &DataType::Utf8)?
Review Comment:
I still wonder if we should allow number other than 0 or allow numbers at all. I guess it's an open question.
I like the jsonpath style with [*] - https://www.rfc-editor.org/rfc/rfc9535.html#section-2.3.2
--
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] [Variant] Support list paths in ShreddedSchemaBuilder [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #10635:
URL: https://github.com/apache/arrow-rs/pull/10635#issuecomment-5698621531
Thanks @cakeni @klion26 :rocket:
--
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] [Variant] Support list paths in ShreddedSchemaBuilder [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #10635:
URL: https://github.com/apache/arrow-rs/pull/10635#issuecomment-6352280345
if its experimental i think it should be fine
--
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] [Variant] Support list paths in ShreddedSchemaBuilder [arrow-rs]
Posted by "cakeni (via GitHub)" <gi...@apache.org>.
cakeni commented on code in PR #10635:
URL: https://github.com/apache/arrow-rs/pull/10635#discussion_r3885157425
##########
parquet-variant-compute/src/shred_variant.rs:
##########
@@ -2898,6 +2911,49 @@ mod tests {
Ok(())
}
+ #[test]
+ fn test_variant_schema_builder_list() -> Result<()> {
+ let shredding_type = ShreddedSchemaBuilder::default()
+ .with_path("items[0].id", &DataType::Int64)?
+ .with_path("items[42].name", &DataType::Utf8)?
Review Comment:
Agreed, `[0]` is confusing here since it already means the first array element in `VariantPath`. I’d prefer `[*]` for the shared element schema and keep numeric indexes unchanged. A separate list API seems harder to compose with nested paths.
--
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] [Variant] Support list paths in ShreddedSchemaBuilder [arrow-rs]
Posted by "cakeni (via GitHub)" <gi...@apache.org>.
cakeni commented on PR #10635:
URL: https://github.com/apache/arrow-rs/pull/10635#issuecomment-5692051034
Updated `[*]` handling to return `InvalidArgumentError`.
--
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] [Variant] Support list paths in ShreddedSchemaBuilder [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on PR #10635:
URL: https://github.com/apache/arrow-rs/pull/10635#issuecomment-5424857439
Sorry for the late reply, a little busy for the inner works these days, will take a look at this tomorrow.
--
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] [Variant] Support list paths in ShreddedSchemaBuilder [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #10635:
URL: https://github.com/apache/arrow-rs/pull/10635#issuecomment-5528182406
> Agreed, [0] is confusing here since it already means the first array element in VariantPath. I’d prefer [*] for the shared element schema and keep numeric indexes unchanged. A separate list API seems harder to compose with nested paths.
Let's proceed with [*] here then. The new `VariantPathElement` can stay NYI for `variant_get`
> Using ['*'] is better than [0]
We should use [*] as ["*"] would be a field name.
--
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] [Variant] Support list paths in ShreddedSchemaBuilder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10635:
URL: https://github.com/apache/arrow-rs/pull/10635#issuecomment-6343673015
@Jefffrey noticed that this PR introduced an API difference (a new public enum value)
- https://github.com/apache/arrow-rs/pull/11750#pullrequestreview-5612965876
Is it ok to release 60.1.0 (a minor version) with this (potentially breaking API change)? Or should we revert this PR / update the enum to avoid breaking?
--
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] [Variant] Support list paths in ShreddedSchemaBuilder [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #10635:
URL: https://github.com/apache/arrow-rs/pull/10635#issuecomment-6344144317
`parquet-variant` is experimental, but we do have downstream consumers.
Only `datafusion-variant` seems to be using arrow-rs 60, but I wouldn't want to introduce breaking changes for anyone else, so I'm okay with reverting.
--
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] [Variant] Support list paths in ShreddedSchemaBuilder [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #10635:
URL: https://github.com/apache/arrow-rs/pull/10635#discussion_r3869374415
##########
parquet-variant-compute/src/shred_variant.rs:
##########
@@ -2898,6 +2911,49 @@ mod tests {
Ok(())
}
+ #[test]
+ fn test_variant_schema_builder_list() -> Result<()> {
+ let shredding_type = ShreddedSchemaBuilder::default()
+ .with_path("items[0].id", &DataType::Int64)?
+ .with_path("items[42].name", &DataType::Utf8)?
Review Comment:
Using `['*']` is better than [0] here from my side because the `variant_get` will receive the `variantpath` and [0] means the first item in the array, and using [0] to indicate the whole array here may be confusing.
Does adding an interface for `ShreddedSchemaBuilder` to support list type solve this(like `VariantBuilder::new_list()`)? But this seems can't handle the case where `with_path(.., ..)` receives a `VariantPath` for the first argument very well(there may be `[]` in the VariantPath), and not sure if it will be too complicated for such a case 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] [Variant] Support list paths in ShreddedSchemaBuilder [arrow-rs]
Posted by "cakeni (via GitHub)" <gi...@apache.org>.
cakeni commented on code in PR #10635:
URL: https://github.com/apache/arrow-rs/pull/10635#discussion_r3775706173
##########
parquet-variant-compute/src/shred_variant.rs:
##########
@@ -2898,6 +2911,49 @@ mod tests {
Ok(())
}
+ #[test]
+ fn test_variant_schema_builder_list() -> Result<()> {
+ let shredding_type = ShreddedSchemaBuilder::default()
+ .with_path("items[0].id", &DataType::Int64)?
+ .with_path("items[42].name", &DataType::Utf8)?
Review Comment:
I agree that arbitrary indexes are a bit misleading here since they all map to the same list element schema.
For this PR, I’d prefer to allow only [0] and reject other indexes. [*] makes sense, but that probably belongs in a separate schema-path/API change.
--
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] [Variant] Support list paths in ShreddedSchemaBuilder [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #10635:
URL: https://github.com/apache/arrow-rs/pull/10635#discussion_r3980950307
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -182,6 +182,9 @@ pub(crate) fn follow_shredded_path_element(
None => Ok(missing_path_step()),
}
}
+ VariantPathElement::ListElement => Err(ArrowError::NotYetImplemented(
Review Comment:
Sounds good. I suggested NYI because I wasn't sure whether we'd want to support this later.
After checking, Spark's `variant_get` doesn't support `[*]` either.
`a.b[*]` selects all elements of the list, while `a.b` selects the whole list itself as one value. Since `variant_get` only support singular paths, I agree that `InvalidArgumentError` is better.
--
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] [Variant] Support list paths in ShreddedSchemaBuilder [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #10635:
URL: https://github.com/apache/arrow-rs/pull/10635#issuecomment-5686896442
@cakeni could you please change the NYI err to `InvalidArgumentError` and this will be good to go.
--
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] [Variant] Support list paths in ShreddedSchemaBuilder [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #10635:
URL: https://github.com/apache/arrow-rs/pull/10635#discussion_r3976889559
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -182,6 +182,9 @@ pub(crate) fn follow_shredded_path_element(
None => Ok(missing_path_step()),
}
}
+ VariantPathElement::ListElement => Err(ArrowError::NotYetImplemented(
Review Comment:
Do we need to implement this in the future? If not, maybe we can use `InvalidArgumentError` or another error type.
The two paths `a.b` and `a.b.[*]` both mean the whole list?
--
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