-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Implement ScalarValue::Map #11224
Implement ScalarValue::Map #11224
Conversation
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.
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 = [ |
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.
👍
@@ -3399,6 +3451,43 @@ impl fmt::Display for ScalarValue { | |||
.join(",") | |||
)? | |||
} | |||
ScalarValue::Map(map_arr) => { | |||
if map_arr.null_count() == map_arr.len() { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I add some tests to cover the Debug function in the latest commit.
datafusion/common/src/scalar/mod.rs
Outdated
@@ -3471,9 +3560,6 @@ impl fmt::Debug for ScalarValue { | |||
ScalarValue::List(_) => write!(f, "List({self})"), | |||
ScalarValue::LargeList(_) => write!(f, "LargeList({self})"), | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this removal intentional?
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.
oops, I removed the wrong lines. I'll revert it. Thanks.
)), | ||
true, | ||
)) | ||
.unwrap(), |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great. I'll add it. Thanks.
Thanks, looks good - I left couple notes but nothing major! |
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!
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
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