[
https://issues.apache.org/jira/browse/ARROW-18274?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17630044#comment-17630044
]
Matthew Topol commented on ARROW-18274:
---------------------------------------
>From what i was able to deduce from your statements, (correct me if I'm wrong)
>but If you use a Struct whose single field is a union containing a Dictionary
>type, you run into a problem? Could you provide a full snippet that hits the
>failure to initialize the Dictionary memo table? I wasn't able to replicate
>the failure to initialize the dictionary memo table you were referring to, but
>I did encounter a different problem when attempting to Print the resulting
>struct array.
Here's my code:
{code:go}
dt1 := arrow.SparseUnionOf([]arrow.Field{
{Name: "c", Type: &arrow.DictionaryType{IndexType:
arrow.PrimitiveTypes.Uint16, ValueType: arrow.BinaryTypes.String}},
}, []arrow.UnionTypeCode{0})
dt2 := arrow.StructOf(arrow.Field{Name: "a", Type: dt1})
pool := memory.NewGoAllocator()
bldr := array.NewStructBuilder(pool, dt2)
defer bldr.Release()
bldrDt1 := bldr.FieldBuilder(0).(*array.SparseUnionBuilder)
binDictBldr := bldrDt1.Child(0).(*array.BinaryDictionaryBuilder)
bldr.Append(true)
bldrDt1.Append(0)
binDictBldr.AppendString("foo")
bldr.Append(true)
bldrDt1.Append(0)
binDictBldr.AppendString("bar")
out := bldr.NewStructArray()
defer out.Release()
fmt.Println(out) // runs into an issue here
{code}
The issue it runs into with that print is in the Struct array. Because a union
array has no null bitmap bytes it attempts to update the field to use the
Struct Array's validity bitmap as a mask, except it calls release on a nil
buffer of the masked field. So there's a need to add a nil check in the Struct
Array {{newStructFieldWithParentValidityMask}} method.
That said, if you can give me a code snippet that replicates the badly
initialized dictionary builder then I'll take a look at that and see where the
issue is.
To address your confusion from the analysis:
{quote} The `NewSparseUnionBuilder` calls the builders for each variant and
also calls defer builder.Release. {quote}
This is because it calls {{NewSparseUnionBuilderWithBuilders}} which delegates
to {{newUnionBuilder}} which will internally call {{Retain}} on each builder.
The reason for this is that you can call {{NewSparseUnionBuilderWithBuilders}}
and provide your own builders to construct a {{SparseUnionBuilder}} with
instead of having it create them for you. In that scenario we have to call
{{Retain}} on each provided builder to ensure the {{SparseUnionBuilder}}
doesn't have them removed from under it. So, when we create the builders
ourselves we have to clean up the extra refcount by calling release *after* we
construct everything and call {{Retain}}.
{quote}The Struct Release method calls the Release methods of every field even
if the refCount is not 0, so the Release method of the second union is called
followed by the Release method of the dictionary. {quote}
If you look at the {{setData}} method on the Struct array, you see that it
constructs an {{arrow.Array}} instance for each child, referencing the
{{ArrayData}} object. Each call to {{NewSliceData}} or {{MakeFromData}} will
call {{Retain}} on the {{ArrayData}} object and produce an {{Array}} that gets
put into the Struct's slice of fields which is not being referenced externally
to the struct (to start with). These can then get referenced externally to the
struct by calling {{Field}} which doesn't automatically call {{Retain}} before
returning (so you don't need to call {{Release}} every time you call {{Field}}.
So, to ensure the fields stay alive as necessary for the struct array, calling
{{Retain}} on the Struct Array will always call {{Retain}} on each child, and
calling {{Release}} on the struct array will always call {{Release}} on each
child. For child fields that are never {{Retain}}'d outside of the Struct
array, the retain and release's will always match, cleaning up as necessary. In
theory a potential improvement might be to have it *not* call {{Retain}} on
each child when you call {{Retain}} on the struct array, and then only call
{{Release}} on the children when the Struct's refCount is 0 after a release.
But as it currently stands, I don't see any situation other than improper usage
that would cause an issue.
Please tag me when you update with a code snippet that exemplifies the failure
you're hitting so i can take a look. In the meantime, I'm going to fix the
issue this found in the Struct's {{String()}} method and file it as a separate
JIRA card.
> [Go] Sparse union of structs is buggy
> -------------------------------------
>
> Key: ARROW-18274
> URL: https://issues.apache.org/jira/browse/ARROW-18274
> Project: Apache Arrow
> Issue Type: Bug
> Components: Go
> Affects Versions: 10.0.0
> Reporter: Laurent Querel
> Priority: Major
>
> Union of structs is currently buggy in V10. See the following example.
>
> {code:go}
> dt1 := arrow.SparseUnionOf([]arrow.Field{
> {Name: "c", Type: &arrow.DictionaryType
> { IndexType: arrow.PrimitiveTypes.Uint16, ValueType:
> arrow.BinaryTypes.String, Ordered: false, }}
> ,
> }, []arrow.UnionTypeCode{0})
> dt2 := arrow.SparseUnionOf([]arrow.Field
> { \{Name: "a", Type: dt1}
> ,
> }, []arrow.UnionTypeCode{0})
> pool := memory.NewGoAllocator()
> array := array.NewSparseUnionBuilder(pool, dt2) {code}
>
> The created array is unusable because the memo table of the dictionary
> builder (field 'c') is nil.
> When I replace the struct by a second union (so 2 nested union), the
> dictionary builder is properly initialized.
>
> *First analysis:*
> - The `NewSparseUnionBuilder` calls the builders for each variant and also
> calls defer builder.Release.
> - The Struct Release method calls the Release methods of every field even if
> the refCount is not 0, so the Release method of the second union is called
> followed by the Release method of the dictionary.
>
> This bug doesn't happen with 2 nested unions as the internal counter is
> properly tested.
>
> In the first place I don't understand why the Release method of each variant
> is call just after the creation of the Union builder. I also don't understand
> why the Release method of the Struct calls the Release method of each field
> independently of the value of the internal refCount.
>
> Any idea?
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
