-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-35740: Add documentation for list arrays' values property #35865
Conversation
|
Thank you for the contribution @spenczar ! I think the main difference between You are correct in the understanding that the behaviour of Some nit for finish: there are doctest errors that need to be fixed with adding
|
Thanks for the explanation @AlenkaF, that makes a lot of sense, and I learned a bunch about Arrow internals from playing with I think I've fixed the doctest issue, too, we'll see if it passes CI now. |
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.
Thanks for the corrections!
Some minor nits ...
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.
Thanks for the PR @spenczar!
I added a few more comments. I think if we expand it even more (eg additional example), we can further clarify the difference between values and flatten(). But note that what you have here is already a clear improvement, so it's certainly fine as well to just focus on finalizing the edits you have now in this PR.
Co-authored-by: Alenka Frim <[email protected]> Co-authored-by: Joris Van den Bossche <[email protected]>
Hello! Anything else to be done here? This is affecting some intersphinx stuff I'm doing in a mild way. |
Is there anything I can do to get this merged? |
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.
Sorry for the delay @spenczar!
I would like us to address the unresolved comment before merging. Otherwise the PR looks good 👍
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 6837d05. There were 108 benchmark results with an error:
There was 1 benchmark result indicating a performance regression:
The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them. |
…pache#35865) Just docs. I'm a little shaky on my understanding of exactly what's going on with FixedSizeListArray.values, and its behavior with nulls, so that wording might deserve a careful read. * Closes: apache#35740 Lead-authored-by: Spencer Nelson <[email protected]> Co-authored-by: Joris Van den Bossche <[email protected]> Co-authored-by: Alenka Frim <[email protected]> Signed-off-by: AlenkaF <[email protected]>
Just docs.
I'm a little shaky on my understanding of exactly what's going on with FixedSizeListArray.values, and its behavior with nulls, so that wording might deserve a careful read.