zeroshade commented on code in PR #926:
URL: https://github.com/apache/arrow-go/pull/926#discussion_r3566723407
##########
arrow/array/data.go:
##########
@@ -187,13 +187,22 @@ func (d *Data) Dictionary() arrow.ArrayData { return
d.dictionary }
// SetDictionary allows replacing the dictionary for this particular Data
object
func (d *Data) SetDictionary(dict arrow.ArrayData) {
- if d.dictionary != nil {
- d.dictionary.Release()
- d.dictionary = nil
+ var newDict *Data
+ if dict != nil {
+ var ok bool
+ newDict, ok = dict.(*Data)
+ if !ok {
+ panic("arrow/array: dictionary data must be
*array.Data")
+ }
}
- if dict.(*Data) != nil {
- dict.Retain()
- d.dictionary = dict.(*Data)
+
+ if newDict != nil {
+ newDict.Retain()
Review Comment:
Retain-before-release ordering is correct. Please add a `SetDictionary(nil)`
test (clearing is now supported per the PR description but untested), and
optionally one asserting the explicit panic for a non-`*array.Data`
implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
