[Variant] Support list paths in ShreddedSchemaBuilder - #10635
Conversation
| let shredding_type = ShreddedSchemaBuilder::default() | ||
| .with_path("items[0].id", &DataType::Int64)? | ||
| .with_path("items[42].name", &DataType::Utf8)? |
There was a problem hiding this 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
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this 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.
|
Sorry for the late reply, a little busy for the inner works these days, will take a look at this tomorrow. |
…-list-paths # Conflicts: # parquet-variant-compute/src/shred_variant.rs
Let's proceed with
We should use |
klion26
left a comment
klion26
left a comment
There was a problem hiding this comment.
Overall, the change LGTM, left one inline comment for the error type.
| None => Ok(missing_path_step()), | ||
| } | ||
| } | ||
| VariantPathElement::ListElement => Err(ArrowError::NotYetImplemented( |
There was a problem hiding this 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?
There was a problem hiding this 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.
3 participants
Which issue does this PR close?
Rationale for this change
ShreddedSchemaBuildercan 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?
ShreddedSchemaBuilder.Are these changes tested?
Yes.
cargo test -p parquet-variant-compute test_variant_schema_builderpasses (11 tests).Are there any user-facing changes?
Yes. Callers can now use indexed paths such as
items[0].idwhen constructing shredding schemas. This is backward compatible.AI assistance
OpenAI Codex assisted with implementation, documentation, and test drafting. I reviewed the resulting design and changes.