-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[Variant] Support creating nested objects and object with lists #7778
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Variant] Support creating nested objects and object with lists #7778
Conversation
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @friendlymatthew -- I went through this carefully and I think it looks great
As you might expect by now I had some suggestions, and I coded them up as another PR for your consideration:
| inner_object_builder.finish(); | ||
| } | ||
|
|
||
| outer_object_builder.finish(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at these examples, it seems to me like the current API requires callingfinish -- it seems like it might be a nicer API if that just happened on drop() -- so you could use the scope of the example, like you have here, and the dropping would just complete the object
This would be a great follow on PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid a clone and add some more comments
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this PR basically follows the existing pattern I am going to merge it in to keep the code flowing;
@PinkCrow007 @scovich @mkarbo @carpecodeum or @Weijun-H -- please feel free to review this PR and comment (or make a PR) for anything else you might find / see could be improved
|
@friendlymatthew Why are objects and lists treated as "pending" and not eagerly added? I am adding a function to parse JSON into Variant, and because of this, I am seeing a behavioral difference between Spark's |
Hi, by eagerly added do you mean to the parent builder? We can't add an object or list to the parent builder's buffer until the object/list itself has been fully created because each nested object/list has its own header and we can't know what the header will be. What sort of behavioral differences are you seeing? If you call |
|
The difference I am seeing is that when creating a Variant out of the JSON string Adding This is not a logical issue by any means though. |
Good catch! I just put up a PR that should fix the issue above. #7786 |
# Rationale for this change A follow up from #7778, we should make sure to check for pending fields before calling `ObjectBuilder::insert`
Which issue does this PR close?
VariantBuilder#7696Rationale for this change
This PR adds a feature to
Variant::ObjectBuilderthat enables constructing nested objects and objects with lists.Are there any user-facing changes?
Adds two public methods to the
ObjectBuilderAPI,new_listandnew_object