Add builder to help create Schemas for shredding (ShreddedSchemaBuilder)#8940
Add builder to help create Schemas for shredding (ShreddedSchemaBuilder)#8940alamb merged 8 commits intoapache:mainfrom
ShreddedSchemaBuilder)#8940Conversation
klion26
left a comment
There was a problem hiding this comment.
Thanks for the contribution, LGTM, left some nit comments to be considered.
| if child_fields.is_empty() { | ||
| None | ||
| } else { | ||
| Some(DataType::Struct(Fields::from(child_fields))) |
There was a problem hiding this comment.
Seems we can construct DataType::Struct and primitive types (use "" as the path), and no List types, not sure if we need to add the information struct information in the builder name or anywhere.
There was a problem hiding this comment.
I am not sure what you mean by this
There was a problem hiding this comment.
ShredTypeBuilder is primarily used for building Struct data types. Since Variants can be shredded as either Structs or Arrays, the previous comment raises the question of whether we should inform users that this cannot be used to build Array schemas.
| } | ||
|
|
||
| #[test] | ||
| fn test_variant_schema_builder_conflicting_path() { |
|
FYI @mhilton -- this PR may be interesting to you |
alamb
left a comment
There was a problem hiding this comment.
Thanks @klion26 and @XiangpengHao -- this is great
The only thing I think we should consider is supporting FieldRefs directly rather than assuming DataType / nullable fields.
I left some other comments that might be useful but are not necessary in my opinion
| if child_fields.is_empty() { | ||
| None | ||
| } else { | ||
| Some(DataType::Struct(Fields::from(child_fields))) |
There was a problem hiding this comment.
I am not sure what you mean by this
| Field::new("age", DataType::Int64, true), | ||
| ])); | ||
| let schema2 = ShredTypeBuilder::default() | ||
| .with_path("id", &DataType::Int32) |
There was a problem hiding this comment.
this certainly is much nicer now
ShredTypeBuilderShredTypeBuilder)
|
FYI @scovich in case you have seen similar APIs or have ideas |
mhilton
left a comment
There was a problem hiding this comment.
I think it would be nice if this builder had a similar structure to VariantBuilder with new_object and new_list. A list can only have one type so it wouldn't work exactly the same though. However such an API and the proposed API aren't mutually exclusive.
|
|
||
| /// Internal tree node structure for building variant schemas. | ||
| #[derive(Clone)] | ||
| enum VariantSchemaNode { |
There was a problem hiding this comment.
It'd be nice if this type also supported an Array type too. Although the path syntax would need to be extended a little. Possibly you could use .0 to indicate an array?
There was a problem hiding this comment.
Arrow List is not supported by shred_variant yet, maybe we can wait until array shredding is implemented
|
Traveling this week but would love to take a look when I get a chance, hopefully next week |
ShredTypeBuilder)ShreddedSchemaBuilder)
|
I just pushed a fix for the test failures https://github.com/apache/arrow-rs/actions/runs/19977045867/job/57295616147 The issue was that I pushed a fix in 1081990 |
|
In terms of List shredding, it seems like perhaps after this PR from @liamzwbao it would be a good follow on project (could someone file a ticket)? |
# Which issue does this PR close? - Related to #8940 # Rationale for this change When reviewing #8940 it wasn't clear you could create a `VariantPath` from a string Thus let's add an example to the documentation # What changes are included in this PR? Add an example to the documentation # Are these changes tested? Yes, by CI # Are there any user-facing changes? Docs only, no functional change
Good catch... I think that was probably my bug 🤦 |
|
Thanks @XiangpengHao @scovich @mhilton and @klion26 -- we are moving this right along again |
Rationale for this change
Basically a helper to simplify this:
What changes are included in this PR?
ShredTypeBuilderAre these changes tested?
Yes
Are there any user-facing changes?
Add a new public interface