Conversation
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @goldmedal - I think this looks great to me
cc @Blizzara what do you think?
|
|
||
| //verify compared to arrow display | ||
| let batch = RecordBatch::try_from_iter(vec![("m", arr as _)]).unwrap(); | ||
| let expected = [ |
| )? | ||
| } | ||
| ScalarValue::Map(map_arr) => { | ||
| if map_arr.null_count() == map_arr.len() { |
There was a problem hiding this comment.
should this also have the assert like Struct?
// ScalarValue Map should always have a single element
assert_eq!(map_arr.len(), 1);
There was a problem hiding this comment.
or if not, then maybe https://github.com/apache/datafusion/pull/11224/files#diff-49e275af8f09685c7bbc491db8ab3b9479960878f42ac558ec0e3e39570590bdR3583 shoulnd't have it either ? 😅
There was a problem hiding this comment.
should this also have the assert like Struct?
// ScalarValue Map should always have a single element assert_eq!(map_arr.len(), 1);
No, MapArray is StructArray. It could contain more than one element.
or if not, then maybe https://github.com/apache/datafusion/pull/11224/files#diff-49e275af8f09685c7bbc491db8ab3b9479960878f42ac558ec0e3e39570590bdR3583 shoulnd't have it either ? 😅
I forgot to remove this. Thanks for reminding me.
There was a problem hiding this comment.
I add some tests to cover the Debug function in the latest commit.
| ScalarValue::Struct(struct_arr) => { | ||
| // ScalarValue Struct should always have a single element | ||
| assert_eq!(struct_arr.len(), 1); | ||
|
|
There was a problem hiding this comment.
was this removal intentional?
There was a problem hiding this comment.
oops, I removed the wrong lines. I'll revert it. Thanks.
| )), | ||
| true, | ||
| )) | ||
| .unwrap(), |
There was a problem hiding this comment.
these would both be NULL maps, right? is it possible to add a test for a map containing some values as well?
There was a problem hiding this comment.
Sounds great. I'll add it. Thanks.
|
Thanks, looks good - I left couple notes but nothing major! |
|
Thanks @goldmedal and @Blizzara 🚀 |
* tmp * introduce ScalarValue::Map * add display test * cargo fmt * address comments and enhance tests
* tmp * introduce ScalarValue::Map * add display test * cargo fmt * address comments and enhance tests
3 participants
Which issue does this PR close?
Closes #11128
Rationale for this change
This PR only implements the Map in ScalarValue. However, we have no corresponding operations for it yet. I think we can implement a new scalar function called make_map() to create this scalar value. I plan to implement it in the next PR.
What changes are included in this PR?
Are these changes tested?
yes
Are there any user-facing changes?
no