-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-10162: [Rust] Add pretty print support for DictionaryArray #8331
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
rust/arrow/src/util/pretty.rs
Outdated
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.
I don't really grok why the error messages refer to repl in this file, but I have carried forward the tradition with this 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.
I think this (pretty printing) was part of DataFusion's cli to begin with but was moved into arrow as it was generally useful. I don't understand repl in this context either. I suggest updating it.
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.
I will update the messages.
rust/arrow/src/util/pretty.rs
Outdated
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.
I made this pub so I could call it from datafusion/src/test/sql.rs which currently has a copy/pasted version of pretty printing in array_str: https://github.com/apache/arrow/blob/master/rust/datafusion/tests/sql.rs#L646
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.
See #8333
rust/arrow/src/util/pretty.rs
Outdated
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.
There is no reason / need to clone to column when printing each value
|
Changes look good to me, but I'm wondering now how the existing code handles null values in the primitive arrays. Seems to me that would print the default value. For Would it make sense to fix this in the same PR or should I open a separate ticket? |
Yes you are correct (that the behavior is incorrect). Good point. I opened https://issues.apache.org/jira/browse/ARROW-10169 and #8332 |
Null values should be printed as "" when pretty printing. Prior to this PR, , null values in primitive arrays were rendered as the type's default value as pointed out by @jhorstmann on #8331 (comment) Closes #8332 from alamb/alamb/ARROW-10169-fix-null-in-pretty-print Authored-by: alamb <[email protected]> Signed-off-by: Jorge C. Leitao <[email protected]>
7201e2a to
d27b581
Compare
|
Rebased to resolve conflicts |
Null values should be printed as "" when pretty printing. Prior to this PR, , null values in primitive arrays were rendered as the type's default value as pointed out by @jhorstmann on apache/arrow#8331 (comment) Closes #8332 from alamb/alamb/ARROW-10169-fix-null-in-pretty-print Authored-by: alamb <[email protected]> Signed-off-by: Jorge C. Leitao <[email protected]>
This adds some logic for handling dictionary arrays when pretty printing batches. Part of a larger set of work I am working on for better DictionaryArray handling: https://issues.apache.org/jira/browse/ARROW-10159