Meaningful error message for map builder with null keys #3450
Conversation
| if !matches!(data.data_type(), DataType::Map(_, _)) { | ||
| return Err(ArrowError::InvalidArgumentError(format!( | ||
| "MapArray expected ArrayData with DataType::Map got {}", | ||
| data.data_type() | ||
| ))); | ||
| } |
There was a problem hiding this comment.
refactoring out a panic into an Err
| assert!( | ||
| keys_arr.null_count() == 0, | ||
| "Keys array must have no null values, found {} null value(s)", | ||
| keys_arr.null_count() | ||
| ); |
There was a problem hiding this comment.
ideally would return an Err instead of panicking, but would change signature of finish & finish_cloned, unsure if that is desirable? especially since other structs like StructBuilder has a similar signature (finish doesn't return a Result, just the StructArray)
There was a problem hiding this comment.
In theory the API for adding keys wouldn't allow adding null values for keys -- but I see that is not possible with the current API
I agree keeping the current (non fallable) API is a reasonable choice
|
cc @tustvold |
| assert!( | ||
| keys_arr.null_count() == 0, | ||
| "Keys array must have no null values, found {} null value(s)", | ||
| keys_arr.null_count() | ||
| ); |
There was a problem hiding this comment.
In theory the API for adding keys wouldn't allow adding null values for keys -- but I see that is not possible with the current API
I agree keeping the current (non fallable) API is a reasonable choice
| self.finish_helper(keys_arr, values_arr, offset_buffer, null_bit_buffer, len) | ||
| } | ||
|
|
||
| fn finish_helper( |
|
Benchmark runs are scheduled for baseline = 7805a81 and contender = 39eeeaf. 39eeeaf is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Follow-on from discussion in PR: #3205 (comment)
Rationale for this change
Have clearer error message for when attempting to finish a MapBuilder which has null keys. Also do some refactoring.
What changes are included in this PR?
Are there any user-facing changes?