Skip to content

feat(extensions): add VariantGet for path extraction from variant arrays - #1206

Open
nssalian wants to merge 3 commits into
apache:mainfrom
nssalian:add-variant-get
Open

feat(extensions): add VariantGet for path extraction from variant arrays#1206
nssalian wants to merge 3 commits into
apache:mainfrom
nssalian:add-variant-get

Conversation

@nssalian

Copy link
Copy Markdown
Contributor

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?

  • Adds VariantGet(input, GetOptions{Path, AsType, Safe, Mem}): extracts a path by following the shredded typed_value columns as far as possible and only reassembling the residual value per-row for the rest. Mirrors the arrow-rs variant_get
  • AsType == nil returns a VariantArray pointing at the path; set it to get a typed array. Also extracts the variant-leaf -> typed-builder cast matrix from shreddedPrimitiveBuilder.tryTyped into a shared appendVariantToTypedBuilder so the writer and VariantGet share it (tryTyped behavior unchanged).

Follow-ups (out of scope here):

  • Array-index columnar push-down - index steps currently use the per-row fallback. The columnar take kernel lives in arrow/compute, which arrow/extensions cannot import (cycle); needs a local gather to push indices into the shredded columns.
  • Lenient cross-type casts - only exact-match + numeric widening are supported; string->bool, numeric->bool, decimal->float, etc. are not ported.
  • Nested AsType output - struct/list target types return arrow.ErrNotImplemented; would need a recursive per-field builder.

Are these changes tested?

  • variant_get_test.go covers 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.go adds a white-box check that the perfect-shredding fast path fires and a CheckedAllocator leak check.

Are there any user-facing changes?

Yes - new exported API: VariantGet, GetOptions, VariantPath, VariantPathElement, VariantPathField, VariantPathIndex. Additive only.

@nssalian
nssalian marked this pull request as ready for review August 16, 2026 01:37
@nssalian
nssalian requested a review from zeroshade as a code owner August 16, 2026 01:37
@nssalian
nssalian marked this pull request as draft August 16, 2026 02:07
@nssalian
nssalian marked this pull request as ready for review August 16, 2026 03:06
Comment thread arrow/extensions/variant_get.go Outdated
Comment thread arrow/extensions/variant_get.go Outdated
Comment thread arrow/extensions/variant_get.go Outdated
Comment thread arrow/extensions/variant_get.go Outdated
Comment thread arrow/extensions/variant_get.go Outdated
Comment thread arrow/extensions/variant_get.go Outdated
Comment thread arrow/extensions/variant_get.go Outdated
Comment thread arrow/extensions/variant_get.go Outdated
Comment thread arrow/extensions/variant_get.go Outdated
Comment on lines +416 to +431
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
}
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do this instead of keeping the bitmap and then using BitmapOr? That would likely be simpler and more performant

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread arrow/extensions/variant_get.go Outdated

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some edge case tests that are missing which should probably get added:

  • Mixed shredded/residual rows at root and nested levels
  • Supported AsType conversion 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

Comment thread arrow/extensions/variant_get.go Outdated
Comment thread arrow/extensions/variant_get.go Outdated
Comment thread arrow/extensions/variant_get.go Outdated
Comment thread arrow/extensions/variant_get.go Outdated
Comment thread arrow/extensions/variant_get.go Outdated
Comment thread arrow/extensions/variant_get.go Outdated
Comment thread arrow/extensions/variant_get.go Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants