feat(extensions): add VariantGet for path extraction from variant arrays - #1206
feat(extensions): add VariantGet for path extraction from variant arrays#1206nssalian wants to merge 3 commits into
Conversation
| func (n *nullTracker) apply(arr arrow.Array) { | ||
| if arr == nil || arr.NullN() == 0 { | ||
| return | ||
| } | ||
| if n.valid == nil { | ||
| n.valid = make([]bool, n.length) | ||
| for i := range n.valid { | ||
| n.valid[i] = true | ||
| } | ||
| } | ||
| for i := 0; i < n.length; i++ { | ||
| if arr.IsNull(i) { | ||
| n.valid[i] = false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
why do this instead of keeping the bitmap and then using BitmapOr? That would likely be simpler and more performant
There was a problem hiding this comment.
Dropped the []bool for a bitmap. Kept as BitmapAndAlloc on validity (1=valid), which is the validity-space equivalent of OR-ing null masks - commented on nullTracker.merge. Happy to spell it as BitmapOr on null-masks if you'd prefer
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Some edge case tests that are missing which should probably get added:
- Mixed shredded/residual rows at root and nested levels
- Supported
AsTypeconversion matrix - Corrupted object metadata versus genuinely missing keys
- Field-on-scalar versus index-on-scalar semantics
- Index greater than
math.MaxUint32 - A missing path with a nested
AsType - Empty-string object key in the path
- Sliced and zero-length inputs
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Found three blocking data-correctness issues: residual-backed rows are lost for non-empty paths, heterogeneous numeric leaves are silently dropped based on row order, and empty-string object keys are encoded as array index zero.
This review was drafted by an AI-assisted tool and confirmed by an Apache Arrow Go maintainer. After you've addressed the points above and pushed an update, an Apache Arrow Go maintainer — a real person — will take the next look at the PR. If you think one of the findings is misapplied, please reply on the PR and a maintainer will weigh in.
More on how Apache Arrow Go handles maintainer review:
CONTRIBUTING.md.
| } | ||
|
|
||
| if step.kind == stepSuccess { | ||
| nulls.merge(state.typedValue) |
There was a problem hiding this comment.
Blocking: This still drops residual-backed rows whenever the path is non-empty. A successful schema-level step merges the parent typed_value validity into the output mask, so a row with null typed_value but a complete value in the residual column is forced null. The new mixed-row test uses an empty path and never executes this line. For $.a, [{"a":1},{"a":2}] with row 2 residual-backed returns [1,null]. Please implement per-row fallback/partitioning and cover root field, nested field, and list-index paths.
| var natural arrow.DataType | ||
| for _, l := range leaves { | ||
| if l.present && l.value.Type() != variant.Null { | ||
| natural = naturalArrowType(l.value) |
There was a problem hiding this comment.
Blocking: Choosing the source type from the first present leaf silently discards later values of another natural width/type at lines 424–425. Variant integers commonly encode as different widths: values 1, 1000, and 5000000000 become int8, int16, and int64; requesting AsType: int64 currently returns [1,null,null]. Results also become row-order-dependent and Strict behaves differently depending on the fast path. Please materialize against the requested target type or otherwise unify leaf types, with mixed-width and mixed-type regression coverage.
|
|
||
| // Field returns a copy of the path with an object-field step appended. | ||
| func (p VariantPath) Field(name string) VariantPath { | ||
| return VariantPath{elems: append(p.grow(), pathElem{name: name})} |
There was a problem hiding this comment.
Blocking: Field("") produces the same zero-valued pathElem as Index(0), and StepAt/GetByPath distinguish steps using name != "". Empty-string object keys are valid, so extracting {"":42} with Field("") is interpreted as array index 0 and returns null. Please retain an explicit field/index discriminator through StepAt and path rebuilding, with an empty-key regression test.
2 participants
Rationale for this change
Reading one field from a variant column reassembles the whole value per row (
VariantArray.Value) then navigates to the field in Go. When the variant is shredded, that field is often already a typed column, so the full reassembly is wasted work.What changes are included in this PR?
VariantGet(input, GetOptions{Path, AsType, Safe, Mem}): extracts a path by following the shreddedtyped_valuecolumns as far as possible and only reassembling the residualvalueper-row for the rest. Mirrors the arrow-rs variant_getAsType == nilreturns aVariantArraypointing at the path; set it to get a typed array. Also extracts the variant-leaf -> typed-builder cast matrix fromshreddedPrimitiveBuilder.tryTypedinto a sharedappendVariantToTypedBuilderso the writer andVariantGetshare it (tryTypedbehavior unchanged).Follow-ups (out of scope here):
takekernel lives inarrow/compute, whicharrow/extensionscannot import (cycle); needs a local gather to push indices into the shredded columns.AsTypeoutput - struct/list target types returnarrow.ErrNotImplemented; would need a recursive per-field builder.Are these changes tested?
variant_get_test.gocovers the extraction paths (typed and variant output, nested path, array index, missing field, fallback, null rows, dictionary-encoded metadata, nested-type rejection).variant_get_internal_test.goadds a white-box check that the perfect-shredding fast path fires and aCheckedAllocatorleak check.Are there any user-facing changes?
Yes - new exported API:
VariantGet,GetOptions,VariantPath,VariantPathElement,VariantPathField,VariantPathIndex. Additive only.