You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nssalian (via GitHub)" <gi...@apache.org> on 2026/07/29 22:57:50 UTC
[PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
nssalian opened a new pull request, #1607:
URL: https://github.com/apache/iceberg-go/pull/1607
Adds predicate pushdown for shredded variant fields and rejects Variant as a partition
source. Closes [#1016](https://github.com/apache/iceberg-go/issues/1016).
## Changes
1. Extract expressions - `extract(variant, path, type)` referencing a field inside a variant column. RFC-9535 path parse (`variant_path.go`), bind against the shredded schema (`variant_extract.go`), cast to the target type (`variant_cast.go`). Follows Java `Expressions.extract` / `PathUtil.parse` / `VariantExpressionUtil.castTo`.
2. File pruning - inclusive/strict metrics evaluators use a shredded field's stored bounds for an extract term (`table/evaluators.go`); the strict path never prunes on extract. Bounds are split via arrow-go `Metadata.SizeBytes()`
(`table/internal/variant_bounds.go`). Follows Java `InclusiveMetricsEvaluator` /`StrictMetricsEvaluator`.
3. Residual filtering (columnar) - surviving extract terms become synthetic derived columns evaluated through the existing arrow-go `compute` record-filter path, not row-by-row (`scanTranslator` in `visitors.go`, `table/variant_residual.go`,`table/arrow_scanner.go`).
4. Reject Variant partition source - `addSpecFieldInternal` (`partitions.go`) rejects a Variant source for any transform. Builds on [#1206](https://github.com/apache/iceberg-go/pull/1206) (which added Variant to `IdentityTransform`'s deny-list); this path does not consult `CanTransform`, so it closes the gap for sources added via `AddPartitionFieldBySourceID`.
## Testing
- Unit tests for path parse, cast, extract binding, residual eval, metrics pruning, and partition rejection; end-to-end shredded-write + pushdown test (`table/variant_shredded_write_test.go`).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #1607:
URL: https://github.com/apache/iceberg-go/pull/1607#discussion_r3899681129
##########
variant_cast.go:
##########
@@ -0,0 +1,269 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package iceberg
+
+import (
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/decimal"
+ "github.com/apache/arrow-go/v18/arrow/decimal128"
+ "github.com/apache/arrow-go/v18/parquet/variant"
+ "github.com/google/uuid"
+)
+
+const (
+ microsPerDay = int64(86_400_000_000)
+ nanosPerDay = int64(86_400_000_000_000)
+ nanosPerMicro = int64(1_000)
+)
+
+// CastVariantLiteral casts a leaf variant value to typ and wraps it as a Literal.
+func CastVariantLiteral(v variant.Value, typ PrimitiveType) (Literal, bool) {
+ result, ok := castVariantValue(v, typ)
+ if !ok {
+ return nil, false
+ }
+
+ lit := literalFromCastValue(result)
+ if lit == nil {
+ return nil, false
+ }
+ if !lit.Type().Equals(typ) {
+ conv, err := lit.To(typ)
+ if err != nil {
+ return nil, false
+ }
+
+ lit = conv
+ }
+
+ return lit, true
+}
+
+// castVariantValue casts a leaf variant value to the Go value backing typ.
+func castVariantValue(v variant.Value, typ PrimitiveType) (any, bool) {
+ raw := v.Value()
+ if raw == nil {
+ return nil, false
+ }
+
+ if r, ok := exactVariantMatch(v.Type(), raw, typ); ok {
+ return r, true
+ }
+
+ switch t := typ.(type) {
+ case Int32Type:
+ switch n := raw.(type) {
+ case int8:
+ return int32(n), true
+ case int16:
+ return int32(n), true
+ }
+ case Int64Type:
+ switch n := raw.(type) {
+ case int8:
+ return int64(n), true
+ case int16:
+ return int64(n), true
+ case int32:
+ return int64(n), true
+ }
+ case Float64Type:
+ if f, ok := raw.(float32); ok {
+ return float64(f), true
+ }
+ case FixedType:
+ if b, ok := raw.([]byte); ok && len(b) == t.Len() {
+ return b, true
+ }
+ case DecimalType:
+ return castVariantDecimal(raw, t)
+ case BooleanType:
+ if b, ok := raw.(bool); ok {
+ return b, true
+ }
+ case TimestampType, TimestampTzType:
Review Comment:
This was a great catch. The nanos/date casts now gate on tz-awareness and only accept the matching physical type, so a zoneless leaf can't land in a tz target. `TestCastVariantTimestampTZ` covers each pair. Also guarded the multiply against int64 overflow while I was in there; it was silently wrapping past ~year 2262.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1607:
URL: https://github.com/apache/iceberg-go/pull/1607#discussion_r3937420488
##########
schema.go:
##########
@@ -524,6 +524,33 @@ func (s *Schema) accessorForField(id int) (accessor, bool) {
return acc, ok
}
+// columnPathSegments returns the own-name path segments from the top-level column down to id, so a name containing '.' stays one segment.
+func (s *Schema) columnPathSegments(id int) []string {
+ idToField, err := s.lazyIDToField()
Review Comment:
**major** — columnPathSegments — the actual dot-in-name fix — is guarded by no test, and the test that appears to guard it bypasses it
The production fix for the dot-in-name bug lives in Schema.columnPathSegments (schema.go:528), which replaced strings.Split(FindColumnName(id), ".") at visitors.go:721. It has zero coverage. The new TestBuildExtractColumnDottedName (table/variant_shredded_write_test.go:1995) constructs `SourcePath: []string{"a.b"}` by hand, so it never calls the producer; it only exercises descendByPath, which is unchanged in this delta. The fix is correct but free to silently regress, and the test name misrepresents what is covered. Ask: add a root-package test asserting columnPathSegments returns ["a.b"] for a top-level field named "a.b" and ["wrap.per","pay.load"] for the nested case (or assert SourcePath through TranslateColumnNamesForScan, which I confirmed yields ["a.b"]).
<details><summary>Evidence</summary>
```text
Mutation 1: loop body -> 'return nil' unconditionally => `go test . -count=1` ok github.com/apache/iceberg-go 0.393s ; `go test ./table/ -count=1` ok github.com/apache/iceberg-go/table 8.266s. Mutation 2: body -> 'strings.Split(s.FindColumnName(id), ".")' (old buggy behavior) => `go test .` ok 0.390s ; `go test ./table/` ok 6.916s. Both files restored; git status --porcelain empty.
```
</details>
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on PR #1607:
URL: https://github.com/apache/iceberg-go/pull/1607#issuecomment-5486817621
Also fixed two issues found while revisiting the diff: a nil-base panic when an `extract` is OR'd with `IsNull` on a column absent from the file, and an `int64` overflow in the Date/timestamp casts past ~year 2262 (now returns not-castable). In addition to that: `appendExtractLiteral` errors instead of panicking, `parseVariantPath` handles surrogate-pair escapes, and the scan tests are consolidated into `variant_shredded_test.go` with some cleanup.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #1607:
URL: https://github.com/apache/iceberg-go/pull/1607#discussion_r3899678722
##########
table/variant_residual.go:
##########
@@ -0,0 +1,193 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "context"
+ "fmt"
+ "strconv"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/compute"
+ "github.com/apache/arrow-go/v18/arrow/extensions"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/iceberg-go"
+ "github.com/google/uuid"
+)
+
+// augmentSchemaWithExtracts returns fileSchema plus one primitive column per variant extract term.
+func augmentSchemaWithExtracts(fileSchema *iceberg.Schema, cols []iceberg.VariantExtractColumn) (*iceberg.Schema, error) {
+ fields := fileSchema.Fields()
+ for _, c := range cols {
+ fields = append(fields, iceberg.NestedField{
+ ID: c.FieldID,
+ Name: c.Name,
+ Type: c.Term.Type().(iceberg.PrimitiveType),
+ })
+ }
+
+ return iceberg.NewSchema(fileSchema.ID, fields...), nil
+}
+
+// buildExtractColumn materializes one variant extract term into a typed Arrow array over rec.
+func buildExtractColumn(col iceberg.VariantExtractColumn, rec arrow.RecordBatch, mem memory.Allocator) (arrow.Array, arrow.Field, error) {
+ typ := col.Term.Type().(iceberg.PrimitiveType)
+ dt, err := TypeToArrowType(typ, false, false)
+ if err != nil {
+ return nil, arrow.Field{}, err
+ }
+
+ bldr := array.NewBuilder(mem, dt)
+ defer bldr.Release()
+
+ n := int(rec.NumRows())
+ varIdx := fieldIndexByID(rec.Schema(), col.Term.Ref().Field().ID)
+ varr, _ := columnAt(rec, varIdx).(*extensions.VariantArray)
+
+ for i := 0; i < n; i++ {
+ if varr == nil || varr.IsNull(i) {
+ bldr.AppendNull()
+
+ continue
+ }
+
+ v, verr := varr.Value(i)
+ if verr != nil {
+ bldr.AppendNull()
Review Comment:
Added a `slog.Warn` for the undecodable-value case so it's not silent. Kept it non-fatal (didn't want one bad row to kill the whole scan) - happy to make it a hard error if you'd prefer.
##########
variant_path.go:
##########
@@ -0,0 +1,132 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package iceberg
+
+import (
+ "fmt"
+ "strings"
+)
+
+// NormalizeVariantPath renders member names as the spec's RFC-9535 normalized JSON path.
+func NormalizeVariantPath(fields []string) string {
+ if len(fields) == 0 {
+ return "$"
+ }
+
+ var b strings.Builder
+ b.WriteByte('$')
+ for _, f := range fields {
+ b.WriteString("['")
+ b.WriteString(rfc9535Escape(f))
+ b.WriteString("']")
+ }
+
+ return b.String()
+}
+
+func rfc9535Escape(name string) string {
+ if strings.IndexFunc(name, func(r rune) bool {
+ return r < 0x20 || r == '\'' || r == '\\'
+ }) < 0 {
+ return name
+ }
+
+ var b strings.Builder
+ b.Grow(len(name) + 4)
+ for _, r := range name {
+ switch r {
+ case '\b':
+ b.WriteString(`\b`)
+ case '\t':
+ b.WriteString(`\t`)
+ case '\f':
+ b.WriteString(`\f`)
+ case '\n':
+ b.WriteString(`\n`)
+ case '\r':
+ b.WriteString(`\r`)
+ case '\'':
+ b.WriteString(`\'`)
+ case '\\':
+ b.WriteString(`\\`)
+ default:
+ if r < 0x20 {
+ fmt.Fprintf(&b, `\u%04x`, r)
+ } else {
+ b.WriteRune(r)
+ }
+ }
+ }
+
+ return b.String()
+}
+
+// parseVariantPath parses a dot-shorthand variant path ($.a.b) into its member names.
+func parseVariantPath(path string) ([]string, error) {
Review Comment:
Fixed - `parseVariantPath` now takes both dot and bracket forms, so `Path()` round-trips. Went a bit further and taught it to decode `\uXXXX` surrogate pairs (astral chars) instead of mangling them to U+FFFD; lone/unpaired surrogates error now. I had a PR for the java one too but it needed some fallback mechanism that was not trivial - will do that later.
##########
table/evaluators.go:
##########
@@ -868,25 +880,44 @@ func (m *inclusiveMetricsEval) VisitNotNan(t iceberg.BoundTerm) bool {
return rowsMightMatch
}
-func (m *inclusiveMetricsEval) VisitLess(t iceberg.BoundTerm, lit iceberg.Literal) bool {
- field := t.Ref().Field()
- fieldID := field.ID
+// boundFor decodes the file bound for term t from raw: a scalar for a reference, or the
Review Comment:
Intentional - a nil bound just means "nothing to prune on", so keeping the file is the right best-effort behavior. Same posture as the extract decode path above.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #1607:
URL: https://github.com/apache/iceberg-go/pull/1607#discussion_r3919463836
##########
table/variant_residual.go:
##########
@@ -0,0 +1,252 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "context"
+ "fmt"
+ "log/slog"
+ "strconv"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/compute"
+ "github.com/apache/arrow-go/v18/arrow/extensions"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/iceberg-go"
+ "github.com/google/uuid"
+)
+
+// augmentSchemaWithExtracts returns fileSchema plus one primitive column per variant extract term.
+func augmentSchemaWithExtracts(fileSchema *iceberg.Schema, cols []iceberg.VariantExtractColumn) *iceberg.Schema {
+ fields := fileSchema.Fields()
+ for _, c := range cols {
+ fields = append(fields, iceberg.NestedField{
+ ID: c.FieldID,
+ Name: c.Name,
+ Type: c.Term.Type().(iceberg.PrimitiveType),
+ })
+ }
+
+ return iceberg.NewSchemaWithIdentifiers(fileSchema.ID, fileSchema.IdentifierFieldIDs, fields...)
+}
+
+// buildExtractColumn materializes one variant extract term into a typed Arrow array over rec.
+func buildExtractColumn(col iceberg.VariantExtractColumn, rec arrow.RecordBatch, mem memory.Allocator) (arrow.Array, arrow.Field, error) {
+ typ := col.Term.Type().(iceberg.PrimitiveType)
+ dt, err := TypeToArrowType(typ, false, false)
+ if err != nil {
+ return nil, arrow.Field{}, err
+ }
+
+ bldr := array.NewBuilder(mem, dt)
+ defer bldr.Release()
+
+ n := int(rec.NumRows())
+ varName := col.Term.Ref().Field().Name
+ varIdx := fieldIndexByID(rec.Schema(), col.Term.Ref().Field().ID)
+ if varIdx < 0 {
+ // Arrow field may lack PARQUET:field_id metadata on name-mapping reads; fall back to the column name.
+ if idxs := rec.Schema().FieldIndices(varName); len(idxs) == 1 {
Review Comment:
Done. I dropped the logical-name fallback entirely. The synthetic column now carries the translated physical field path (SourcePath, derived from fileSchema.FindColumnName in extractRef), and buildExtractColumn resolves the source by field id (descending nested structs) with the physical path as the id-less fallback. Rename and struct-nested cases both resolve now; an unresolved source still errors cleanly instead of null-filling. Covered by TestBuildExtractColumnRenamedSource and TestBuildExtractColumnNested.
re the "nested containers" - I checked whether an extract can even bind to a variant nested in a list/map: Reference.Bind rejects it with "invalid schema" (same as any list/map-element ref, so it's the existing binding behavior, not extract-specific), which means those never reach materialization. So struct descent covers everything that can actually bind. Left it struct-only for that reason - happy to extend the descent to list/map if you'd rather have it explicit.
##########
visitors.go:
##########
@@ -822,6 +901,11 @@ func (sanitizeVisitor) VisitOr(left, right BooleanExpression) BooleanExpression
}
func (sanitizeVisitor) VisitUnbound(pred UnboundPredicate) BooleanExpression {
+ // An extract term has no REST expression-JSON form; keeping it would fail json.Marshal. Collapse it like bbox (mirrors VisitBound).
+ if _, ok := pred.Term().(*unboundExtract); ok {
+ return AlwaysTrue{}
Review Comment:
Good catch - gave it the same polarity protection as stripExtractPredicates. sanitizeVisitor now returns a sanitizedResult carrying a hasUnserializable flag (set for extract and bbox), OR-propagated through And/Or, and VisitNot collapses a subtree containing one to AlwaysTrue instead of NewNot(AlwaysTrue) = AlwaysFalse. So the ScanReport no longer misstates the filter. TestSanitizeExpressionExtractCollapses now asserts the NOT(extract) case (unbound + bound) resolves to AlwaysTrue and still marshals.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1607:
URL: https://github.com/apache/iceberg-go/pull/1607#discussion_r3732110360
##########
variant_cast.go:
##########
@@ -0,0 +1,269 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package iceberg
+
+import (
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/decimal"
+ "github.com/apache/arrow-go/v18/arrow/decimal128"
+ "github.com/apache/arrow-go/v18/parquet/variant"
+ "github.com/google/uuid"
+)
+
+const (
+ microsPerDay = int64(86_400_000_000)
+ nanosPerDay = int64(86_400_000_000_000)
+ nanosPerMicro = int64(1_000)
+)
+
+// CastVariantLiteral casts a leaf variant value to typ and wraps it as a Literal.
+func CastVariantLiteral(v variant.Value, typ PrimitiveType) (Literal, bool) {
+ result, ok := castVariantValue(v, typ)
+ if !ok {
+ return nil, false
+ }
+
+ lit := literalFromCastValue(result)
+ if lit == nil {
+ return nil, false
+ }
+ if !lit.Type().Equals(typ) {
+ conv, err := lit.To(typ)
+ if err != nil {
+ return nil, false
+ }
+
+ lit = conv
+ }
+
+ return lit, true
+}
+
+// castVariantValue casts a leaf variant value to the Go value backing typ.
+func castVariantValue(v variant.Value, typ PrimitiveType) (any, bool) {
+ raw := v.Value()
+ if raw == nil {
+ return nil, false
+ }
+
+ if r, ok := exactVariantMatch(v.Type(), raw, typ); ok {
+ return r, true
+ }
+
+ switch t := typ.(type) {
+ case Int32Type:
+ switch n := raw.(type) {
+ case int8:
+ return int32(n), true
+ case int16:
+ return int32(n), true
+ }
+ case Int64Type:
+ switch n := raw.(type) {
+ case int8:
+ return int64(n), true
+ case int16:
+ return int64(n), true
+ case int32:
+ return int64(n), true
+ }
+ case Float64Type:
+ if f, ok := raw.(float32); ok {
+ return float64(f), true
+ }
+ case FixedType:
+ if b, ok := raw.([]byte); ok && len(b) == t.Len() {
+ return b, true
+ }
+ case DecimalType:
+ return castVariantDecimal(raw, t)
+ case BooleanType:
+ if b, ok := raw.(bool); ok {
+ return b, true
+ }
+ case TimestampType, TimestampTzType:
Review Comment:
**Blocking: this conflates tz-aware and zoneless timestamps, and it produces wrong rows rather than just wrong pruning.**
`exactVariantMatch` (`variant_cast.go:62-84`) gets this right — `TimestampMicrosNTZ` -> `timestamp`, `TimestampMicros` -> `timestamptz`. But the fallback dispatch here drops the distinction entirely: both `TimestampType` and `TimestampTzType` route to the same `castVariantToMicros`, and both `TimestampNsType` and `TimestampTzNsType` route to the same `castVariantToNanos`. `castVariantToMicros` (`:195-227`) then accepts both `variant.TimestampNanos` *and* `variant.TimestampNanosNTZ` regardless of which target was requested.
Net effect: `Extract(p, "$.ts", TimestampTz)` silently matches a zoneless nanosecond leaf, treating local time as UTC, and `Extract(p, "$.ts", Timestamp)` silently matches a tz-aware nanosecond leaf. Since this feeds both file pruning and the residual filter, the rows that come back are wrong. The inconsistency with `exactVariantMatch` reads as an oversight rather than a decision.
Suggested fix: pass the target's tz-awareness into the three helpers and require the source physical type to agree.
```go
func castVariantToMicros(pt variant.Type, raw any, tz bool) (any, bool) {
switch {
case tz && pt == variant.TimestampNanos, !tz && pt == variant.TimestampNanosNTZ:
return Timestamp(floorDiv(int64(raw.(arrow.Timestamp)), nanosPerMicro)), true
...
```
##########
table/variant_residual.go:
##########
@@ -0,0 +1,193 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "context"
+ "fmt"
+ "strconv"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/compute"
+ "github.com/apache/arrow-go/v18/arrow/extensions"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/iceberg-go"
+ "github.com/google/uuid"
+)
+
+// augmentSchemaWithExtracts returns fileSchema plus one primitive column per variant extract term.
+func augmentSchemaWithExtracts(fileSchema *iceberg.Schema, cols []iceberg.VariantExtractColumn) (*iceberg.Schema, error) {
+ fields := fileSchema.Fields()
+ for _, c := range cols {
+ fields = append(fields, iceberg.NestedField{
+ ID: c.FieldID,
+ Name: c.Name,
+ Type: c.Term.Type().(iceberg.PrimitiveType),
+ })
+ }
+
+ return iceberg.NewSchema(fileSchema.ID, fields...), nil
+}
+
+// buildExtractColumn materializes one variant extract term into a typed Arrow array over rec.
+func buildExtractColumn(col iceberg.VariantExtractColumn, rec arrow.RecordBatch, mem memory.Allocator) (arrow.Array, arrow.Field, error) {
+ typ := col.Term.Type().(iceberg.PrimitiveType)
+ dt, err := TypeToArrowType(typ, false, false)
+ if err != nil {
+ return nil, arrow.Field{}, err
+ }
+
+ bldr := array.NewBuilder(mem, dt)
+ defer bldr.Release()
+
+ n := int(rec.NumRows())
+ varIdx := fieldIndexByID(rec.Schema(), col.Term.Ref().Field().ID)
+ varr, _ := columnAt(rec, varIdx).(*extensions.VariantArray)
Review Comment:
**Blocking: the discarded `ok` turns a lookup miss into a silently emptied batch.**
If `fieldIndexByID` misses (returns -1) or the column comes back as something other than a `*extensions.VariantArray`, `varr` is nil, the loop at `:72` appends null for *every* row, and `payload.a = 5` then evaluates false everywhere. That is a wrong-results failure wearing the costume of an empty result — nothing errors and nothing logs.
`varIdx == -1` in particular is reachable when the arrow field lacks `PARQUET:field_id` metadata (name-mapping reads), which is precisely the schema-evolution case where "no rows" is indistinguishable from a legitimate answer.
Suggested fix: distinguish the two cases. Return an explicit error when the column is present but is not a `VariantArray`, and handle "column absent from this file" the way `scanTranslator.VisitBound` already does for a missing column — `AlwaysFalse` for comparison ops, `AlwaysTrue` for `IsNull` — rather than fabricating an all-null column.
##########
variant_extract.go:
##########
@@ -0,0 +1,248 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package iceberg
+
+import (
+ "fmt"
+
+ "github.com/apache/arrow-go/v18/parquet/variant"
+ "github.com/google/uuid"
+)
+
+// BoundExtract is a bound variant sub-path term used for metrics pruning and residual evaluation.
+type BoundExtract interface {
+ BoundTerm
+
+ Path() string
+ // ExtractValue navigates v to this term's path and casts the leaf to the target type.
+ ExtractValue(v variant.Value) (Literal, bool)
+}
+
+// Extract creates an unbound variant sub-path term for a dotted JSONPath.
+func Extract(ref Reference, path string, typ PrimitiveType) UnboundTerm {
+ return &unboundExtract{ref: ref, path: path, typ: typ}
+}
+
+type unboundExtract struct {
Review Comment:
**Blocking: extract terms serialize to `"term":{}` with no error, corrupting REST scan-planning filters.**
Neither `unboundExtract` nor `boundExtract[T]` has a `MarshalJSON`. `marshalLiteralPredicate` (`expr_json.go:230-239`) calls `json.Marshal(term)`, and a struct with only unexported fields marshals to `{}`. So a row filter containing an extract, sent through server-side scan planning (`catalog/rest/scan_planning.go:389`), is encoded as `{"type":"eq","term":{},"value":5}` and shipped to the catalog with no client-side error. There is also no `decodeTerm` case, so it cannot round-trip back.
The repo already has the right precedent for a non-serializable term — `expr_json.go:204-205` returns `ErrBBoxNotSerializable`.
Suggested fix:
```go
func (u *unboundExtract) MarshalJSON() ([]byte, error) {
return nil, fmt.Errorf("%w: variant extract terms have no REST expression form", ErrNotImplemented)
}
func (b *boundExtract[T]) MarshalJSON() ([]byte, error) { /* same */ }
```
##########
variant_path.go:
##########
@@ -0,0 +1,132 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package iceberg
+
+import (
+ "fmt"
+ "strings"
+)
+
+// NormalizeVariantPath renders member names as the spec's RFC-9535 normalized JSON path.
+func NormalizeVariantPath(fields []string) string {
+ if len(fields) == 0 {
+ return "$"
+ }
+
+ var b strings.Builder
+ b.WriteByte('$')
+ for _, f := range fields {
+ b.WriteString("['")
+ b.WriteString(rfc9535Escape(f))
+ b.WriteString("']")
+ }
+
+ return b.String()
+}
+
+func rfc9535Escape(name string) string {
+ if strings.IndexFunc(name, func(r rune) bool {
+ return r < 0x20 || r == '\'' || r == '\\'
+ }) < 0 {
+ return name
+ }
+
+ var b strings.Builder
+ b.Grow(len(name) + 4)
+ for _, r := range name {
+ switch r {
+ case '\b':
+ b.WriteString(`\b`)
+ case '\t':
+ b.WriteString(`\t`)
+ case '\f':
+ b.WriteString(`\f`)
+ case '\n':
+ b.WriteString(`\n`)
+ case '\r':
+ b.WriteString(`\r`)
+ case '\'':
+ b.WriteString(`\'`)
+ case '\\':
+ b.WriteString(`\\`)
+ default:
+ if r < 0x20 {
+ fmt.Fprintf(&b, `\u%04x`, r)
+ } else {
+ b.WriteRune(r)
+ }
+ }
+ }
+
+ return b.String()
+}
+
+// parseVariantPath parses a dot-shorthand variant path ($.a.b) into its member names.
+func parseVariantPath(path string) ([]string, error) {
Review Comment:
Non-blocking round-trip asymmetry: `parseVariantPath` rejects bracket notation, but `BoundExtract.Path()` (`variant_extract.go:146`) *emits* it. So `Extract(ref, term.Path(), typ)` cannot round-trip a path that normalizes to brackets. It also makes field names containing `.`, quotes, or a leading digit unreachable, which Java's `PathUtil.parse` supports.
Suggested fix: accept `$['a']` on input even if array indices stay unsupported for now.
##########
table/evaluators.go:
##########
@@ -868,25 +880,44 @@ func (m *inclusiveMetricsEval) VisitNotNan(t iceberg.BoundTerm) bool {
return rowsMightMatch
}
-func (m *inclusiveMetricsEval) VisitLess(t iceberg.BoundTerm, lit iceberg.Literal) bool {
- field := t.Ref().Field()
- fieldID := field.ID
+// boundFor decodes the file bound for term t from raw: a scalar for a reference, or the
Review Comment:
Non-blocking, just flagging an unremarked behavior change: the non-primitive `panic` guard in `boundFor` now fires only when `raw != nil`, whereas previously it fired unconditionally in `VisitLess`, `VisitIn`, `VisitStartsWith`, and friends. That is more permissive and is very likely fine, but it is not mentioned anywhere in the PR description.
Suggested fix: confirm the loosening is intended and note it in the description (or in a comment here).
##########
table/evaluators.go:
##########
@@ -868,25 +880,44 @@ func (m *inclusiveMetricsEval) VisitNotNan(t iceberg.BoundTerm) bool {
return rowsMightMatch
}
-func (m *inclusiveMetricsEval) VisitLess(t iceberg.BoundTerm, lit iceberg.Literal) bool {
- field := t.Ref().Field()
- fieldID := field.ID
+// boundFor decodes the file bound for term t from raw: a scalar for a reference, or the
+// variant sub-path value for an extract; ok is false when raw is nil or not castable.
+func (m *inclusiveMetricsEval) boundFor(t iceberg.BoundTerm, raw []byte) (iceberg.Literal, bool) {
+ if raw == nil {
+ return nil, false
+ }
- if m.containsNullsOnly(fieldID) || m.containsNansOnly(fieldID) {
- return rowsCannotMatch
+ if ext, ok := t.(iceberg.BoundExtract); ok {
+ lit, found, err := internal.VariantBoundLiteral(raw, ext.Path(), ext.Type().(iceberg.PrimitiveType))
+ if err != nil {
+ panic(err)
Review Comment:
Non-blocking: `panic(err)` turns an undecodable bound into a hard scan error — for example a bound written by another engine, or a `TestRowGroup` path where `lowerBounds[-1]` holds arbitrary variant-leaf stats. Metrics pruning should be best-effort; a bound we cannot decode means we cannot prune, not that the scan should fail.
Worth noting the round trip is self-defeating as written: `VariantBoundLiteral` installs a `recover()` (`table/internal/variant_bounds.go:317-321`) specifically to convert arrow-go panics into errors, and `boundFor` immediately re-panics them.
Suggested fix: return `rowsMightMatch` here instead of panicking.
##########
table/variant_residual.go:
##########
@@ -0,0 +1,193 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "context"
+ "fmt"
+ "strconv"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/compute"
+ "github.com/apache/arrow-go/v18/arrow/extensions"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/iceberg-go"
+ "github.com/google/uuid"
+)
+
+// augmentSchemaWithExtracts returns fileSchema plus one primitive column per variant extract term.
+func augmentSchemaWithExtracts(fileSchema *iceberg.Schema, cols []iceberg.VariantExtractColumn) (*iceberg.Schema, error) {
+ fields := fileSchema.Fields()
+ for _, c := range cols {
+ fields = append(fields, iceberg.NestedField{
+ ID: c.FieldID,
+ Name: c.Name,
+ Type: c.Term.Type().(iceberg.PrimitiveType),
+ })
+ }
+
+ return iceberg.NewSchema(fileSchema.ID, fields...), nil
+}
+
+// buildExtractColumn materializes one variant extract term into a typed Arrow array over rec.
+func buildExtractColumn(col iceberg.VariantExtractColumn, rec arrow.RecordBatch, mem memory.Allocator) (arrow.Array, arrow.Field, error) {
+ typ := col.Term.Type().(iceberg.PrimitiveType)
+ dt, err := TypeToArrowType(typ, false, false)
+ if err != nil {
+ return nil, arrow.Field{}, err
+ }
+
+ bldr := array.NewBuilder(mem, dt)
+ defer bldr.Release()
+
+ n := int(rec.NumRows())
+ varIdx := fieldIndexByID(rec.Schema(), col.Term.Ref().Field().ID)
+ varr, _ := columnAt(rec, varIdx).(*extensions.VariantArray)
+
+ for i := 0; i < n; i++ {
+ if varr == nil || varr.IsNull(i) {
+ bldr.AppendNull()
+
+ continue
+ }
+
+ v, verr := varr.Value(i)
+ if verr != nil {
+ bldr.AppendNull()
Review Comment:
Non-blocking: a variant decode failure (`verr != nil`) is silently converted into "no match" for that row. That is a different thing from the value being absent, and the caller has no way to tell them apart.
Suggested fix: at minimum log it; ideally surface it, since a decode failure usually means the file is malformed rather than that the path is missing.
##########
table/variant_residual.go:
##########
@@ -0,0 +1,193 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "context"
+ "fmt"
+ "strconv"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/compute"
+ "github.com/apache/arrow-go/v18/arrow/extensions"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/iceberg-go"
+ "github.com/google/uuid"
+)
+
+// augmentSchemaWithExtracts returns fileSchema plus one primitive column per variant extract term.
+func augmentSchemaWithExtracts(fileSchema *iceberg.Schema, cols []iceberg.VariantExtractColumn) (*iceberg.Schema, error) {
Review Comment:
Minor: `augmentSchemaWithExtracts` declares an `error` return it never produces — every caller has to handle an impossible case.
Suggested fix: drop the return value. Separately, `iceberg.NewSchema(fileSchema.ID, fields...)` on `:45` drops the source schema's identifier field IDs; harmless for binding today, but easy to preserve while you are here.
##########
variant_path.go:
##########
@@ -0,0 +1,132 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package iceberg
+
+import (
+ "fmt"
+ "strings"
+)
+
+// NormalizeVariantPath renders member names as the spec's RFC-9535 normalized JSON path.
+func NormalizeVariantPath(fields []string) string {
Review Comment:
Minor API-surface note: `NormalizeVariantPath` and `CastVariantLiteral` (`variant_cast.go:41`) appear to be exported only so `table/internal` can reach them. That permanently widens the root package's public surface to serve an internal need.
Suggested fix: consider moving `VariantBoundLiteral`'s decode into the root package instead, or at minimum document both as internal-use and not covered by compatibility guarantees.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1607:
URL: https://github.com/apache/iceberg-go/pull/1607#discussion_r3935764418
##########
table/variant_residual.go:
##########
@@ -0,0 +1,316 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "context"
+ "fmt"
+ "log/slog"
+ "strconv"
+ "strings"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/compute"
+ "github.com/apache/arrow-go/v18/arrow/extensions"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/iceberg-go"
+ "github.com/google/uuid"
+)
+
+// augmentSchemaWithExtracts returns fileSchema plus one primitive column per variant extract term.
+func augmentSchemaWithExtracts(fileSchema *iceberg.Schema, cols []iceberg.VariantExtractColumn) *iceberg.Schema {
+ fields := fileSchema.Fields()
+ for _, c := range cols {
+ fields = append(fields, iceberg.NestedField{
+ ID: c.FieldID,
+ Name: c.Name,
+ Type: c.Term.Type().(iceberg.PrimitiveType),
+ })
+ }
+
+ return iceberg.NewSchemaWithIdentifiers(fileSchema.ID, fileSchema.IdentifierFieldIDs, fields...)
+}
+
+// buildExtractColumn materializes one variant extract term into a typed Arrow array over rec.
+func buildExtractColumn(col iceberg.VariantExtractColumn, rec arrow.RecordBatch, mem memory.Allocator) (arrow.Array, arrow.Field, error) {
+ typ := col.Term.Type().(iceberg.PrimitiveType)
+ dt, err := TypeToArrowType(typ, false, false)
+ if err != nil {
+ return nil, arrow.Field{}, err
+ }
+
+ bldr := array.NewBuilder(mem, dt)
+ defer bldr.Release()
+
+ n := int(rec.NumRows())
+ varName := col.Term.Ref().Field().Name
+ arr := resolveVariantSource(rec, col.Term.Ref().Field().ID, col.SourcePath)
+ if arr == nil {
+ return nil, arrow.Field{}, fmt.Errorf("%w: variant extract column %q not found in file", iceberg.ErrInvalidArgument, varName)
+ }
+ varr, ok := arr.(*extensions.VariantArray)
+ if !ok {
+ return nil, arrow.Field{}, fmt.Errorf("%w: variant extract column %q is not a VariantArray (got %T)", iceberg.ErrInvalidArgument, varName, arr)
+ }
+
+ for i := range n {
+ if varr.IsNull(i) {
+ bldr.AppendNull()
+
+ continue
+ }
+
+ v, verr := varr.Value(i)
+ if verr != nil {
+ slog.Warn("variant extract: skipping undecodable variant value", "column", varName, "row", i, "err", verr)
+ bldr.AppendNull()
+
+ continue
+ }
+
+ lit, ok := col.Term.ExtractValue(v)
+ if !ok {
+ bldr.AppendNull()
+
+ continue
+ }
+
+ if aerr := appendExtractLiteral(bldr, lit); aerr != nil {
+ return nil, arrow.Field{}, aerr
+ }
+ }
+
+ field := arrow.Field{
+ Name: col.Name,
+ Type: dt,
+ Nullable: true,
+ Metadata: arrow.NewMetadata([]string{ArrowParquetFieldIDKey}, []string{strconv.Itoa(col.FieldID)}),
+ }
+
+ return bldr.NewArray(), field, nil
+}
+
+// resolveVariantSource locates the extract's source array by field id, descending
+// nested structs; it falls back to the file-schema path when field ids are absent.
+func resolveVariantSource(rec arrow.RecordBatch, fieldID int, sourcePath string) arrow.Array {
+ for i, f := range rec.Schema().Fields() {
+ if a := descendByFieldID(f, rec.Column(i), fieldID); a != nil {
+ return a
+ }
+ }
+ if sourcePath == "" {
+ return nil
+ }
+
+ return descendByPath(rec.Schema(), rec.Columns(), strings.Split(sourcePath, "."))
+}
+
+func descendByFieldID(f arrow.Field, col arrow.Array, fieldID int) arrow.Array {
+ if v, ok := f.Metadata.GetValue(ArrowParquetFieldIDKey); ok {
+ if id, err := strconv.Atoi(v); err == nil && id == fieldID {
+ return col
+ }
Review Comment:
**minor** — Nested field-id descent is not pinned by any test and appears redundant
Deleting the entire struct-descent loop in descendByFieldID leaves the full table package green, including TestBuildExtractColumnNested whose assertion message claims 'nested variant resolved by descending the struct' but which actually resolves via the descendByPath fallback. Because fileSchema is derived from the same pruned Arrow schema the record batches carry, the path walk always aligns, so the field-id recursion has no exercised code path. Either add a test that only the field-id descent can satisfy (field ids present, path unresolvable) or drop the recursion and keep descendByPath.
##########
visitors.go:
##########
@@ -802,54 +884,79 @@ const sanitizedLiteralMask = "(redacted)"
// predicates (IN / NOT IN) keep their arity so the operation is not
// misrepresented, but the members are masked.
func SanitizeExpression(expr BooleanExpression) (BooleanExpression, error) {
- return VisitExpr(expr, sanitizeVisitor{})
+ res, err := VisitExpr(expr, sanitizeVisitor{})
+ if err != nil {
+ return nil, err
+ }
+
+ return res.expr, nil
+}
+
+// sanitizedResult is the masked expression plus whether its subtree held a non-serializable term.
+type sanitizedResult struct {
+ expr BooleanExpression
+ hasUnserializable bool
Review Comment:
**nit** — NOT over a mixed subtree discards the serializable conjuncts from the ScanReport filter
VisitNot collapses the whole subtree to AlwaysTrue whenever any descendant was unserializable, so NOT(AND(extract=5, x<10)) reports AlwaysTrue and the surviving x<10 never reaches ScanReport.Filter. The direction is safe (over-broad, never AlwaysFalse) and matches the bbox convention, but the emitted report can under-report the executed filter. Worth a one-line comment on the tradeoff, or push the collapse down to the offending leaf and negate the rest.
##########
table/variant_residual.go:
##########
@@ -0,0 +1,316 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "context"
+ "fmt"
+ "log/slog"
+ "strconv"
+ "strings"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/compute"
+ "github.com/apache/arrow-go/v18/arrow/extensions"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/iceberg-go"
+ "github.com/google/uuid"
+)
+
+// augmentSchemaWithExtracts returns fileSchema plus one primitive column per variant extract term.
+func augmentSchemaWithExtracts(fileSchema *iceberg.Schema, cols []iceberg.VariantExtractColumn) *iceberg.Schema {
+ fields := fileSchema.Fields()
+ for _, c := range cols {
+ fields = append(fields, iceberg.NestedField{
+ ID: c.FieldID,
+ Name: c.Name,
+ Type: c.Term.Type().(iceberg.PrimitiveType),
+ })
+ }
+
+ return iceberg.NewSchemaWithIdentifiers(fileSchema.ID, fileSchema.IdentifierFieldIDs, fields...)
+}
+
+// buildExtractColumn materializes one variant extract term into a typed Arrow array over rec.
+func buildExtractColumn(col iceberg.VariantExtractColumn, rec arrow.RecordBatch, mem memory.Allocator) (arrow.Array, arrow.Field, error) {
+ typ := col.Term.Type().(iceberg.PrimitiveType)
+ dt, err := TypeToArrowType(typ, false, false)
+ if err != nil {
+ return nil, arrow.Field{}, err
+ }
+
+ bldr := array.NewBuilder(mem, dt)
+ defer bldr.Release()
+
+ n := int(rec.NumRows())
+ varName := col.Term.Ref().Field().Name
+ arr := resolveVariantSource(rec, col.Term.Ref().Field().ID, col.SourcePath)
+ if arr == nil {
+ return nil, arrow.Field{}, fmt.Errorf("%w: variant extract column %q not found in file", iceberg.ErrInvalidArgument, varName)
+ }
+ varr, ok := arr.(*extensions.VariantArray)
+ if !ok {
+ return nil, arrow.Field{}, fmt.Errorf("%w: variant extract column %q is not a VariantArray (got %T)", iceberg.ErrInvalidArgument, varName, arr)
+ }
+
+ for i := range n {
+ if varr.IsNull(i) {
+ bldr.AppendNull()
+
+ continue
+ }
+
+ v, verr := varr.Value(i)
+ if verr != nil {
+ slog.Warn("variant extract: skipping undecodable variant value", "column", varName, "row", i, "err", verr)
+ bldr.AppendNull()
+
+ continue
+ }
+
+ lit, ok := col.Term.ExtractValue(v)
+ if !ok {
+ bldr.AppendNull()
+
+ continue
+ }
+
+ if aerr := appendExtractLiteral(bldr, lit); aerr != nil {
+ return nil, arrow.Field{}, aerr
+ }
+ }
+
+ field := arrow.Field{
+ Name: col.Name,
+ Type: dt,
+ Nullable: true,
+ Metadata: arrow.NewMetadata([]string{ArrowParquetFieldIDKey}, []string{strconv.Itoa(col.FieldID)}),
+ }
+
+ return bldr.NewArray(), field, nil
+}
+
+// resolveVariantSource locates the extract's source array by field id, descending
+// nested structs; it falls back to the file-schema path when field ids are absent.
+func resolveVariantSource(rec arrow.RecordBatch, fieldID int, sourcePath string) arrow.Array {
+ for i, f := range rec.Schema().Fields() {
+ if a := descendByFieldID(f, rec.Column(i), fieldID); a != nil {
+ return a
+ }
+ }
+ if sourcePath == "" {
+ return nil
+ }
Review Comment:
**minor** — strings.Split on '.' misresolves Variant columns whose Iceberg name contains a dot
resolveVariantSource re-splits the flattened column name returned by FindColumnName, so a legal Iceberg column literally named 'a.b' is looked up as top-level 'a' then child 'b'. On a name-mapped (field-id-less) file this aborts the scan. Resolve by walking the Iceberg schema's nested field structure (carrying the path segments through from FindColumnName) instead of re-splitting the flattened name.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #1607:
URL: https://github.com/apache/iceberg-go/pull/1607#discussion_r3899680013
##########
table/evaluators.go:
##########
@@ -868,25 +880,44 @@ func (m *inclusiveMetricsEval) VisitNotNan(t iceberg.BoundTerm) bool {
return rowsMightMatch
}
-func (m *inclusiveMetricsEval) VisitLess(t iceberg.BoundTerm, lit iceberg.Literal) bool {
- field := t.Ref().Field()
- fieldID := field.ID
+// boundFor decodes the file bound for term t from raw: a scalar for a reference, or the
+// variant sub-path value for an extract; ok is false when raw is nil or not castable.
+func (m *inclusiveMetricsEval) boundFor(t iceberg.BoundTerm, raw []byte) (iceberg.Literal, bool) {
+ if raw == nil {
+ return nil, false
+ }
- if m.containsNullsOnly(fieldID) || m.containsNansOnly(fieldID) {
- return rowsCannotMatch
+ if ext, ok := t.(iceberg.BoundExtract); ok {
+ lit, found, err := internal.VariantBoundLiteral(raw, ext.Path(), ext.Type().(iceberg.PrimitiveType))
+ if err != nil {
+ panic(err)
Review Comment:
Done for extracts - `boundFor` now returns `(nil, false)` on a decode error so pruning stays best-effort, no panic. Left the scalar-reference panic as-is since it's pre-existing and out of scope for this PR
##########
variant_extract.go:
##########
@@ -0,0 +1,248 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package iceberg
+
+import (
+ "fmt"
+
+ "github.com/apache/arrow-go/v18/parquet/variant"
+ "github.com/google/uuid"
+)
+
+// BoundExtract is a bound variant sub-path term used for metrics pruning and residual evaluation.
+type BoundExtract interface {
+ BoundTerm
+
+ Path() string
+ // ExtractValue navigates v to this term's path and casts the leaf to the target type.
+ ExtractValue(v variant.Value) (Literal, bool)
+}
+
+// Extract creates an unbound variant sub-path term for a dotted JSONPath.
+func Extract(ref Reference, path string, typ PrimitiveType) UnboundTerm {
+ return &unboundExtract{ref: ref, path: path, typ: typ}
+}
+
+type unboundExtract struct {
Review Comment:
Now, both `unboundExtract` and `boundExtract` now have `MarshalJSON` returning `ErrExtractNotSerializable`, so it errors instead of shipping `{}`. Checked that the marshal sites (expr_json, REST scan-planning, codec) all propagate it. Asserted in `expr_json_test.go`.
##########
table/variant_residual.go:
##########
@@ -0,0 +1,193 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "context"
+ "fmt"
+ "strconv"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/compute"
+ "github.com/apache/arrow-go/v18/arrow/extensions"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/iceberg-go"
+ "github.com/google/uuid"
+)
+
+// augmentSchemaWithExtracts returns fileSchema plus one primitive column per variant extract term.
+func augmentSchemaWithExtracts(fileSchema *iceberg.Schema, cols []iceberg.VariantExtractColumn) (*iceberg.Schema, error) {
+ fields := fileSchema.Fields()
+ for _, c := range cols {
+ fields = append(fields, iceberg.NestedField{
+ ID: c.FieldID,
+ Name: c.Name,
+ Type: c.Term.Type().(iceberg.PrimitiveType),
+ })
+ }
+
+ return iceberg.NewSchema(fileSchema.ID, fields...), nil
+}
+
+// buildExtractColumn materializes one variant extract term into a typed Arrow array over rec.
+func buildExtractColumn(col iceberg.VariantExtractColumn, rec arrow.RecordBatch, mem memory.Allocator) (arrow.Array, arrow.Field, error) {
+ typ := col.Term.Type().(iceberg.PrimitiveType)
+ dt, err := TypeToArrowType(typ, false, false)
+ if err != nil {
+ return nil, arrow.Field{}, err
+ }
+
+ bldr := array.NewBuilder(mem, dt)
+ defer bldr.Release()
+
+ n := int(rec.NumRows())
+ varIdx := fieldIndexByID(rec.Schema(), col.Term.Ref().Field().ID)
+ varr, _ := columnAt(rec, varIdx).(*extensions.VariantArray)
Review Comment:
Done. A missing column now returns an error, and the `*VariantArray` assertion uses `, ok` and errors instead of leaving `varr` nil, so it can't null out the whole batch. Testing is covered by `TestBuildExtractColumnNameFallback` and `TestVariantExtractOrAbsentColumnNoPanic`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #1607:
URL: https://github.com/apache/iceberg-go/pull/1607#discussion_r3937015307
##########
visitors.go:
##########
@@ -802,54 +884,79 @@ const sanitizedLiteralMask = "(redacted)"
// predicates (IN / NOT IN) keep their arity so the operation is not
// misrepresented, but the members are masked.
func SanitizeExpression(expr BooleanExpression) (BooleanExpression, error) {
- return VisitExpr(expr, sanitizeVisitor{})
+ res, err := VisitExpr(expr, sanitizeVisitor{})
+ if err != nil {
+ return nil, err
+ }
+
+ return res.expr, nil
+}
+
+// sanitizedResult is the masked expression plus whether its subtree held a non-serializable term.
+type sanitizedResult struct {
+ expr BooleanExpression
+ hasUnserializable bool
Review Comment:
Added the one-line note - the collapse is over-broad (drops the serializable siblings from the report) but never AlwaysFalse, matching the bbox convention.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #1607:
URL: https://github.com/apache/iceberg-go/pull/1607#discussion_r3899677715
##########
variant_path.go:
##########
@@ -0,0 +1,132 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package iceberg
+
+import (
+ "fmt"
+ "strings"
+)
+
+// NormalizeVariantPath renders member names as the spec's RFC-9535 normalized JSON path.
+func NormalizeVariantPath(fields []string) string {
Review Comment:
Left these exported for now - they're used by both the root package and `table/internal`, so they can't move into `internal` or go unexported without root depending on the subpackage. Kept the "not part of the stable public API" note. Can revisit in a follow-up if you want them tucked somewhere shared.
##########
table/variant_residual.go:
##########
@@ -0,0 +1,193 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "context"
+ "fmt"
+ "strconv"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/compute"
+ "github.com/apache/arrow-go/v18/arrow/extensions"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/iceberg-go"
+ "github.com/google/uuid"
+)
+
+// augmentSchemaWithExtracts returns fileSchema plus one primitive column per variant extract term.
+func augmentSchemaWithExtracts(fileSchema *iceberg.Schema, cols []iceberg.VariantExtractColumn) (*iceberg.Schema, error) {
Review Comment:
Both done - dropped the error return, and switched to `NewSchemaWithIdentifiers` so the identifier field IDs survive
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #1607:
URL: https://github.com/apache/iceberg-go/pull/1607#discussion_r3937010144
##########
table/variant_residual.go:
##########
@@ -0,0 +1,316 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "context"
+ "fmt"
+ "log/slog"
+ "strconv"
+ "strings"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/compute"
+ "github.com/apache/arrow-go/v18/arrow/extensions"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/iceberg-go"
+ "github.com/google/uuid"
+)
+
+// augmentSchemaWithExtracts returns fileSchema plus one primitive column per variant extract term.
+func augmentSchemaWithExtracts(fileSchema *iceberg.Schema, cols []iceberg.VariantExtractColumn) *iceberg.Schema {
+ fields := fileSchema.Fields()
+ for _, c := range cols {
+ fields = append(fields, iceberg.NestedField{
+ ID: c.FieldID,
+ Name: c.Name,
+ Type: c.Term.Type().(iceberg.PrimitiveType),
+ })
+ }
+
+ return iceberg.NewSchemaWithIdentifiers(fileSchema.ID, fileSchema.IdentifierFieldIDs, fields...)
+}
+
+// buildExtractColumn materializes one variant extract term into a typed Arrow array over rec.
+func buildExtractColumn(col iceberg.VariantExtractColumn, rec arrow.RecordBatch, mem memory.Allocator) (arrow.Array, arrow.Field, error) {
+ typ := col.Term.Type().(iceberg.PrimitiveType)
+ dt, err := TypeToArrowType(typ, false, false)
+ if err != nil {
+ return nil, arrow.Field{}, err
+ }
+
+ bldr := array.NewBuilder(mem, dt)
+ defer bldr.Release()
+
+ n := int(rec.NumRows())
+ varName := col.Term.Ref().Field().Name
+ arr := resolveVariantSource(rec, col.Term.Ref().Field().ID, col.SourcePath)
+ if arr == nil {
+ return nil, arrow.Field{}, fmt.Errorf("%w: variant extract column %q not found in file", iceberg.ErrInvalidArgument, varName)
+ }
+ varr, ok := arr.(*extensions.VariantArray)
+ if !ok {
+ return nil, arrow.Field{}, fmt.Errorf("%w: variant extract column %q is not a VariantArray (got %T)", iceberg.ErrInvalidArgument, varName, arr)
+ }
+
+ for i := range n {
+ if varr.IsNull(i) {
+ bldr.AppendNull()
+
+ continue
+ }
+
+ v, verr := varr.Value(i)
+ if verr != nil {
+ slog.Warn("variant extract: skipping undecodable variant value", "column", varName, "row", i, "err", verr)
+ bldr.AppendNull()
+
+ continue
+ }
+
+ lit, ok := col.Term.ExtractValue(v)
+ if !ok {
+ bldr.AppendNull()
+
+ continue
+ }
+
+ if aerr := appendExtractLiteral(bldr, lit); aerr != nil {
+ return nil, arrow.Field{}, aerr
+ }
+ }
+
+ field := arrow.Field{
+ Name: col.Name,
+ Type: dt,
+ Nullable: true,
+ Metadata: arrow.NewMetadata([]string{ArrowParquetFieldIDKey}, []string{strconv.Itoa(col.FieldID)}),
+ }
+
+ return bldr.NewArray(), field, nil
+}
+
+// resolveVariantSource locates the extract's source array by field id, descending
+// nested structs; it falls back to the file-schema path when field ids are absent.
+func resolveVariantSource(rec arrow.RecordBatch, fieldID int, sourcePath string) arrow.Array {
+ for i, f := range rec.Schema().Fields() {
+ if a := descendByFieldID(f, rec.Column(i), fieldID); a != nil {
+ return a
+ }
+ }
+ if sourcePath == "" {
+ return nil
+ }
+
+ return descendByPath(rec.Schema(), rec.Columns(), strings.Split(sourcePath, "."))
+}
+
+func descendByFieldID(f arrow.Field, col arrow.Array, fieldID int) arrow.Array {
+ if v, ok := f.Metadata.GetValue(ArrowParquetFieldIDKey); ok {
+ if id, err := strconv.Atoi(v); err == nil && id == fieldID {
+ return col
+ }
Review Comment:
Dropped the recursion - resolveVariantSource now does a top-level field-id match then descendByPath for nesting, since the recursion wasn't covering anything the path walk didn't. Added TestBuildExtractColumnResolvesByFieldID (ids present, path deliberately mismatched) so the id branch is actually pinned now.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #1607:
URL: https://github.com/apache/iceberg-go/pull/1607#discussion_r3937012732
##########
table/variant_residual.go:
##########
@@ -0,0 +1,316 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "context"
+ "fmt"
+ "log/slog"
+ "strconv"
+ "strings"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/compute"
+ "github.com/apache/arrow-go/v18/arrow/extensions"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/iceberg-go"
+ "github.com/google/uuid"
+)
+
+// augmentSchemaWithExtracts returns fileSchema plus one primitive column per variant extract term.
+func augmentSchemaWithExtracts(fileSchema *iceberg.Schema, cols []iceberg.VariantExtractColumn) *iceberg.Schema {
+ fields := fileSchema.Fields()
+ for _, c := range cols {
+ fields = append(fields, iceberg.NestedField{
+ ID: c.FieldID,
+ Name: c.Name,
+ Type: c.Term.Type().(iceberg.PrimitiveType),
+ })
+ }
+
+ return iceberg.NewSchemaWithIdentifiers(fileSchema.ID, fileSchema.IdentifierFieldIDs, fields...)
+}
+
+// buildExtractColumn materializes one variant extract term into a typed Arrow array over rec.
+func buildExtractColumn(col iceberg.VariantExtractColumn, rec arrow.RecordBatch, mem memory.Allocator) (arrow.Array, arrow.Field, error) {
+ typ := col.Term.Type().(iceberg.PrimitiveType)
+ dt, err := TypeToArrowType(typ, false, false)
+ if err != nil {
+ return nil, arrow.Field{}, err
+ }
+
+ bldr := array.NewBuilder(mem, dt)
+ defer bldr.Release()
+
+ n := int(rec.NumRows())
+ varName := col.Term.Ref().Field().Name
+ arr := resolveVariantSource(rec, col.Term.Ref().Field().ID, col.SourcePath)
+ if arr == nil {
+ return nil, arrow.Field{}, fmt.Errorf("%w: variant extract column %q not found in file", iceberg.ErrInvalidArgument, varName)
+ }
+ varr, ok := arr.(*extensions.VariantArray)
+ if !ok {
+ return nil, arrow.Field{}, fmt.Errorf("%w: variant extract column %q is not a VariantArray (got %T)", iceberg.ErrInvalidArgument, varName, arr)
+ }
+
+ for i := range n {
+ if varr.IsNull(i) {
+ bldr.AppendNull()
+
+ continue
+ }
+
+ v, verr := varr.Value(i)
+ if verr != nil {
+ slog.Warn("variant extract: skipping undecodable variant value", "column", varName, "row", i, "err", verr)
+ bldr.AppendNull()
+
+ continue
+ }
+
+ lit, ok := col.Term.ExtractValue(v)
+ if !ok {
+ bldr.AppendNull()
+
+ continue
+ }
+
+ if aerr := appendExtractLiteral(bldr, lit); aerr != nil {
+ return nil, arrow.Field{}, aerr
+ }
+ }
+
+ field := arrow.Field{
+ Name: col.Name,
+ Type: dt,
+ Nullable: true,
+ Metadata: arrow.NewMetadata([]string{ArrowParquetFieldIDKey}, []string{strconv.Itoa(col.FieldID)}),
+ }
+
+ return bldr.NewArray(), field, nil
+}
+
+// resolveVariantSource locates the extract's source array by field id, descending
+// nested structs; it falls back to the file-schema path when field ids are absent.
+func resolveVariantSource(rec arrow.RecordBatch, fieldID int, sourcePath string) arrow.Array {
+ for i, f := range rec.Schema().Fields() {
+ if a := descendByFieldID(f, rec.Column(i), fieldID); a != nil {
+ return a
+ }
+ }
+ if sourcePath == "" {
+ return nil
+ }
Review Comment:
Fixed this. This now carries structured path segments from the schema (columnPathSegments walks the nested fields) instead of re-splitting the flattened name, so a column literally named a.b stays one segment. Added TestBuildExtractColumnDottedName.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1607:
URL: https://github.com/apache/iceberg-go/pull/1607#discussion_r3909527617
##########
table/variant_residual.go:
##########
@@ -0,0 +1,252 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "context"
+ "fmt"
+ "log/slog"
+ "strconv"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/compute"
+ "github.com/apache/arrow-go/v18/arrow/extensions"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+ "github.com/apache/iceberg-go"
+ "github.com/google/uuid"
+)
+
+// augmentSchemaWithExtracts returns fileSchema plus one primitive column per variant extract term.
+func augmentSchemaWithExtracts(fileSchema *iceberg.Schema, cols []iceberg.VariantExtractColumn) *iceberg.Schema {
+ fields := fileSchema.Fields()
+ for _, c := range cols {
+ fields = append(fields, iceberg.NestedField{
+ ID: c.FieldID,
+ Name: c.Name,
+ Type: c.Term.Type().(iceberg.PrimitiveType),
+ })
+ }
+
+ return iceberg.NewSchemaWithIdentifiers(fileSchema.ID, fileSchema.IdentifierFieldIDs, fields...)
+}
+
+// buildExtractColumn materializes one variant extract term into a typed Arrow array over rec.
+func buildExtractColumn(col iceberg.VariantExtractColumn, rec arrow.RecordBatch, mem memory.Allocator) (arrow.Array, arrow.Field, error) {
+ typ := col.Term.Type().(iceberg.PrimitiveType)
+ dt, err := TypeToArrowType(typ, false, false)
+ if err != nil {
+ return nil, arrow.Field{}, err
+ }
+
+ bldr := array.NewBuilder(mem, dt)
+ defer bldr.Release()
+
+ n := int(rec.NumRows())
+ varName := col.Term.Ref().Field().Name
+ varIdx := fieldIndexByID(rec.Schema(), col.Term.Ref().Field().ID)
+ if varIdx < 0 {
+ // Arrow field may lack PARQUET:field_id metadata on name-mapping reads; fall back to the column name.
+ if idxs := rec.Schema().FieldIndices(varName); len(idxs) == 1 {
Review Comment:
This fallback does not work after a name-mapped column rename. For an ID-less file, the Arrow batch retains the legacy physical name (for example, `old_payload`), while `col.Term.Ref().Field().Name` is the current logical name (`payload`), so the scan aborts instead of evaluating the extract. This resolution is also top-level-only, so a Variant nested in a struct cannot be found/materialized. Please carry or derive the translated physical field path and descend through nested containers rather than falling back only to the current top-level name.
##########
visitors.go:
##########
@@ -822,6 +901,11 @@ func (sanitizeVisitor) VisitOr(left, right BooleanExpression) BooleanExpression
}
func (sanitizeVisitor) VisitUnbound(pred UnboundPredicate) BooleanExpression {
+ // An extract term has no REST expression-JSON form; keeping it would fail json.Marshal. Collapse it like bbox (mirrors VisitBound).
+ if _, ok := pred.Term().(*unboundExtract); ok {
+ return AlwaysTrue{}
Review Comment:
The unsupported-term collapse needs the same polarity protection as `stripExtractPredicates`. Today `NOT(extract(...) = 1)` visits the extract as `AlwaysTrue` and then becomes `AlwaysFalse`, so the emitted `ScanReport` misstates the filter even though the scan returns rows. Please track whether the child contained an extract/bbox and make a `NOT` over such a subtree conservatively `AlwaysTrue` (or cause the report filter to be omitted).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on PR #1607:
URL: https://github.com/apache/iceberg-go/pull/1607#issuecomment-5488809988
Pushed the fix for the negated-extract pruning blocker - a NOT over an extract now collapses to AlwaysTrue (and hasExtract propagates through AND/OR, so nested NOTs are covered), so negation can't fold the pruning filter into a lossy AlwaysFalse. Added a regression test as well - NOT(Eq(Extract("payload","$.a",Int64),1)) with a payload.a=2 row, asserting it
survives - plus coverage for the required-variant-column bind, the tz/zoneless cast rejection, and missing/wrong-type residual columns. Also strips extracts before TranslateColumnNames so the incremental-scan path no longer hits ErrNotImplemented.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #1607:
URL: https://github.com/apache/iceberg-go/pull/1607#discussion_r3937599266
##########
schema.go:
##########
@@ -524,6 +524,33 @@ func (s *Schema) accessorForField(id int) (accessor, bool) {
return acc, ok
}
+// columnPathSegments returns the own-name path segments from the top-level column down to id, so a name containing '.' stays one segment.
+func (s *Schema) columnPathSegments(id int) []string {
+ idToField, err := s.lazyIDToField()
Review Comment:
Added TestColumnPathSegments in the root package asserting columnPathSegments returns ["a.b"] for a top-level a.b and ["wrap.per","pay.load"] for the nested case.
re the Lossy NOT sanitization in visitors.go: Kept this as the documented tradeoff for now - the collapse is over-broad but never AlwaysFalse, and it only affects the ScanReport filter string (never query results), so I'd rather not re-architect the sanitizer for a report-only nit.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): variant predicate pushdown and reject variant partition source [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1607:
URL: https://github.com/apache/iceberg-go/pull/1607
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org