implement display for ColumnarValue #14220
Conversation
|
cc: @jayzhan211 |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you so much for this contributon @zjregee -- it looks very close
I think we need to fix the panics in the code, but otherwise it looks great
I also took the liberty to push some tests of this code and merge up from main. Thanks again!
|
|
||
| // Implement Display trait for ColumnarValue | ||
| // | ||
| // # Panics |
There was a problem hiding this comment.
In general I think it would be surprising of a Display impl panicd.
Instead in this case perhaps we can convert the errors to a string
One way to do that is instead of
pretty_format_columns("ColumnarValue(ArrayRef)", &[Arc::clone(array)])
.unwrap()You can fallback on error like this:
if let Ok(formatted) = pretty_format_columns("ColumnarValue(ArrayRef)", &[Arc::clone(array)]) {
write!(f, "{}", formatted)
} else {
write!(f, "Error formatting columnar value")
}There was a problem hiding this comment.
Thanks for your help and suggestions, I have fixed this.
| "+----------------------------+\n\ | ||
| | ColumnarValue(ScalarValue) |\n\ | ||
| +----------------------------+\n\ | ||
| | foo |\n\ | ||
| +----------------------------+" |
There was a problem hiding this comment.
I don't quite understand the meaning of align here.
There was a problem hiding this comment.
I mean every line here doesn't seem to be aligned?
ideally:
"+----------------------------+\n\
| ColumnarValue(ScalarValue) |\n\
+----------------------------+\n\
| foo |\n\
+----------------------------+"
...
There was a problem hiding this comment.
Got this, thanks and updated.
| let column = ColumnarValue::from(ScalarValue::from("foo")); | ||
| assert_eq!( | ||
| column.to_string(), | ||
| concat!( |
There was a problem hiding this comment.
❤️ -- TIL "concat!" -- that looks much nicer
4 participants
Which issue does this PR close?
Closes #14176.
Rationale for this change
What changes are included in this PR?
Add a unified presentation for ColumnarValue.
Are these changes tested?
Yes.
Are there any user-facing changes?
None.