-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[Variant] Add tests for variant_get requesting Some struct #8343
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
Conversation
|
CC @alamb @codephage2020 -- another quickie (once the PR it depends on merges) |
klion26
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.
LGTM
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.
Does the comment dummy value for row 2 here mean that row 2 is inner field null?
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.
Does this(row 3) mean writer did not respect the variant spec?
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.
on null rows, the array can contain any physically valid value; it will anyway be ignored
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.
(and they have to contain something to keep row counts in sync)
bf83990 to
7328d19
Compare
codephage2020
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.
Looks good to me ! Thank you !
|
@mbrobbel -- any reason not to merge this? (I don't have rights) |
|
woohoo we are cooking the last few days in arrow-rs |
Which issue does this PR close?
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.
Rationale for this change
variant_gethas code to support reading a struct from perfectly shredded variant data, but no test coverage.What changes are included in this PR?
Add the missing coverage.
Are these changes tested?
Yes
Are there any user-facing changes?
No