Add DictionaryArray Constructors (#3879) #4068
Conversation
github-actions
bot
added
arrow
tustvold
force-pushed
the
dictionary-try-new
branch
from
9212997 to
9c0cbdf
Compare
tustvold
force-pushed
the
dictionary-try-new
branch
from
9c0cbdf to
992a196
Compare
There was a problem hiding this comment.
It looks good to me -- I worry it will be disrupive on downstream crates -- I wonder if we could mark the function deprecated for a release cycle (and maybe call the new DictionaryArray::try_new something bad like DictionaryArray::try_new2)? Maybe it is over thinking it I am just thinking of having to change a bunch of APIs downstream
| @@ -1057,23 +1096,23 @@ mod tests { | |||
|
|
|||
| #[test] | |||
| #[should_panic( | |||
| expected = "Value at position 1 out of bounds: 3 (should be in [0, 1])" | |||
| expected = "Invalid dictionary key 3 at index 1, expected 0 <= key < 2" | |||
There was a problem hiding this comment.
that is certainly a nicer error message
| // Safety: `validate` ensures key type is correct, and | ||
| // `validate_values` ensures all offsets are within range | ||
| let array = unsafe { builder.build_unchecked() }; | ||
| if let Some((idx, v)) = keys.values().iter().enumerate().find(|(idx, v)| { |
There was a problem hiding this comment.
Isn't this code now redundant with the validity checks in ArrayData?
|
IMO marking it deprecated and removing it is only meaningful if there is a tangible behaviour change that is liable to cause people to defer making the change. As it stands, changing the arguments to be owned is likely as much a change as adding My vote is just to get it over and done with, but I will leave this open to see if anybody else weighs in |
Sounds good to me |
Which issue does this PR close?
Part of #3879
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?
Yes, this makes a breaking change to
DictionaryArray::try_newto make it consistent with the other arrays which take owned arguments