laskoviymishka commented on code in PR #1546: URL: https://github.com/apache/iceberg-go/pull/1546#discussion_r3681186893
########## bound_set_literal_view.go: ########## @@ -0,0 +1,49 @@ +// 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 + +func (s readOnlyLiteralSet) Members() []Literal { + members := s.Set.Members() + for i, literal := range members { + members[i] = cloneBoundLiteral(literal) + } + + return members +} + +func (s readOnlyLiteralSet) All(fn func(Literal) bool) bool { + return s.Set.All(func(literal Literal) bool { + return fn(cloneBoundLiteral(literal)) + }) +} + +type boundSetLiteralRef interface { + boundSetLiteralsRef() Set[Literal] +} + +func (bsp *boundSetPredicate[T]) boundSetLiteralsRef() Set[Literal] { + return bsp.lits +} + +func boundSetLiteralsForVisit(predicate BoundPredicate) Set[Literal] { + if ref, ok := predicate.(boundSetLiteralRef); ok { + return ref.boundSetLiteralsRef() + } + + return predicate.(BoundSetPredicate).Literals() Review Comment: The fallback assertion is unguarded, so a custom `BoundPredicate` carrying `OpIn`/`OpNotIn` without implementing `BoundSetPredicate` panics here with a bare `interface conversion: ...`. It can only reach here via `VisitBoundPredicate`, which is recoverable, but the message is opaque. I'd go comma-ok and panic with a wrapped `ErrNotImplemented` naming the op and concrete type. ########## visitors.go: ########## @@ -134,9 +134,9 @@ func visitBoolExpr[T any](e BooleanExpression, visitor BooleanExprVisitor[T]) T func VisitBoundPredicate[T any](e BoundPredicate, visitor BoundBooleanExprVisitor[T]) T { switch e.Op() { case OpIn: - return visitor.VisitIn(e.Term(), e.(BoundSetPredicate).Literals()) + return visitor.VisitIn(e.Term(), boundSetLiteralsForVisit(e)) Review Comment: `VisitBoundPredicate` routes through the fast path now, but `columnNameTranslator.VisitBound` and `sanitizeVisitor.VisitBound` have their own type switch and still call `p.Literals().Members()` directly, so they clone every byte-backed literal. `TranslateColumnNames` runs per data file on schema-evolution scans, so that's the same per-file O(N) clone regression from last round, just on a different path than the metrics eval that was benchmarked. Either thread `boundSetLiteralsForVisit` through here too, or if the clone is genuinely required because the `AsUnbound` recipient may mutate the returned slice, I'd drop a comment saying so. wdyt? ########## exprs_test.go: ########## @@ -472,6 +473,88 @@ func TestInNotInSimplifications(t *testing.T) { assert.True(t, bsp.Literals().Contains(iceberg.NewLiteral("world"))) }) + t.Run("bound literals are isolated", func(t *testing.T) { + isin := iceberg.IsIn(iceberg.Reference("foo"), "hello", "world") + bound, err := isin.(iceberg.UnboundPredicate).Bind(tableSchemaSimple, true) + require.NoError(t, err) + + bsp := bound.(iceberg.BoundSetPredicate) + literals := bsp.Literals() + assert.PanicsWithValue(t, "cannot add to read-only literal set", func() { + literals.Add(iceberg.NewLiteral("injected")) + }) + + assert.Equal(t, 2, bsp.Literals().Len()) + assert.False(t, bsp.Literals().Contains(iceberg.NewLiteral("injected"))) + }) + + t.Run("bound literal values are isolated", func(t *testing.T) { + geometry, err := iceberg.GeometryTypeOf("srid:4326") + require.NoError(t, err) + + tests := []struct { + name string + typ iceberg.Type + lits func([]byte, []byte) []iceberg.Literal + }{ + { + name: "binary", + typ: iceberg.PrimitiveTypes.Binary, + lits: func(first, second []byte) []iceberg.Literal { + return []iceberg.Literal{iceberg.NewLiteral(first), iceberg.NewLiteral(second)} + }, + }, + { + name: "fixed", + typ: iceberg.FixedTypeOf(16), + lits: func(first, second []byte) []iceberg.Literal { + return []iceberg.Literal{iceberg.NewLiteral(first), iceberg.NewLiteral(second)} + }, + }, + { + name: "geometry", + typ: geometry, + lits: func(first, second []byte) []iceberg.Literal { + firstLit, err := iceberg.LiteralFromBytes(geometry, first) + require.NoError(t, err) + secondLit, err := iceberg.LiteralFromBytes(geometry, second) + require.NoError(t, err) + + return []iceberg.Literal{firstLit, secondLit} + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + first := []byte("0123456789abcdef") + second := []byte("fedcba9876543210") + expected := slices.Clone(first) + isin := iceberg.SetPredicate(iceberg.OpIn, iceberg.Reference("value"), tt.lits(first, second)) + schema := iceberg.NewSchema(1, iceberg.NestedField{ID: 1, Name: "value", Type: tt.typ}) + + bound, err := isin.(iceberg.UnboundPredicate).Bind(schema, true) + require.NoError(t, err) + first[0] = 0xff + + expectedLiteral, err := iceberg.LiteralFromBytes(tt.typ, expected) + require.NoError(t, err) + assert.True(t, bound.(iceberg.BoundSetPredicate).Literals().Contains(expectedLiteral)) + }) + } + }) + + t.Run("access bound literals without allocating", func(t *testing.T) { + isin := iceberg.IsIn(iceberg.Reference("foo"), "hello", "world") + bound, err := isin.(iceberg.UnboundPredicate).Bind(tableSchemaSimple, true) + require.NoError(t, err) + bsp := bound.(iceberg.BoundSetPredicate) + + assert.Zero(t, testing.AllocsPerRun(100, func() { Review Comment: This asserts `bsp.Literals()` doesn't allocate, which is trivially true since `literalView` is pre-stored — it doesn't cover the thing that actually matters, which is `boundSetLiteralsForVisit(pred).Contains(val)` staying zero-alloc end to end. I'd point `AllocsPerRun` at a `VisitBoundPredicate` call with a small `BoundBooleanExprVisitor` stub that calls `Contains` in `VisitIn`, so the test guards the regression that was actually flagged. wdyt? ########## bound_set_literal_view_test.go: ########## @@ -0,0 +1,100 @@ +// 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_test + +import ( + "slices" + "testing" + + iceberg "github.com/apache/iceberg-go" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestBoundSetPredicateLiteralViewClonesMutableMembers(t *testing.T) { + t.Parallel() + + geometry, err := iceberg.GeometryTypeOf("srid:4326") + require.NoError(t, err) + + tests := []struct { + name string + typ iceberg.Type + }{ + {name: "binary", typ: iceberg.PrimitiveTypes.Binary}, + {name: "fixed", typ: iceberg.FixedTypeOf(16)}, + {name: "geometry", typ: geometry}, Review Comment: `GeographyType` binds to `[]byte` and produces a `GeoLiteral` through the exact same clone branch as geometry, but neither this table nor the sibling in `exprs_test.go` exercises it. Low risk since it's the same path, but a `{name: "geography", typ: iceberg.GeographyTypeOf("srid:4326")}` row confirms the wiring end to end. Worth adding while we're here. ########## bound_set_literal_view.go: ########## @@ -0,0 +1,49 @@ +// 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 + +func (s readOnlyLiteralSet) Members() []Literal { + members := s.Set.Members() + for i, literal := range members { + members[i] = cloneBoundLiteral(literal) + } + + return members +} + +func (s readOnlyLiteralSet) All(fn func(Literal) bool) bool { + return s.Set.All(func(literal Literal) bool { + return fn(cloneBoundLiteral(literal)) + }) +} + +type boundSetLiteralRef interface { + boundSetLiteralsRef() Set[Literal] +} + +func (bsp *boundSetPredicate[T]) boundSetLiteralsRef() Set[Literal] { + return bsp.lits +} + +func boundSetLiteralsForVisit(predicate BoundPredicate) Set[Literal] { Review Comment: This is the clever bit that keeps the hot path zero-alloc, and for our built-in visitors it's safe since they only ever call `Contains`/`Len`/`All`. But it also hands the raw, mutable `bsp.lits` straight to every `VisitIn`/`VisitNotIn` — including third-party `BoundBooleanExprVisitor` impls, who have no signal that calling `Add` on the `Set[Literal]` they were given mutates the predicate's internal state and aliases the cloned bytes. So the wrapper closes the hole for `Literals()` callers and the bypass reopens it for visitor callers. I'd either narrow the bypass to the read-only surface the built-ins actually need, or document on `BoundBooleanExprVisitor.VisitIn`/`VisitNotIn` that the `Set[Literal]` must be treated as read-only. Leaning toward the doc contract since it's cheap — how do you want to frame it? ########## exprs.go: ########## @@ -1057,7 +1062,38 @@ func (bsp *boundSetPredicate[T]) AsUnbound(r Reference, lits []Literal) UnboundP } func (bsp *boundSetPredicate[T]) Literals() Set[Literal] { - return bsp.lits + return bsp.literalView +} + +type readOnlyLiteralSet struct { + Set[Literal] +} + +func (readOnlyLiteralSet) Add(...Literal) { + panic("cannot add to read-only literal set") Review Comment: Every other deliberate panic in this package goes through `panic(fmt.Errorf("%w: ...", ErrXxx))`, so `VisitExpr`'s recover can type-check it and `errors.Is` works. This one's a bare string, so a recover lands in `case string` and loses the error type. I'd make it a package sentinel (`fmt.Errorf("%w: cannot add to read-only literal set", ErrInvalidArgument)`) and panic that, then switch the test to `PanicsWithError`. ########## exprs.go: ########## @@ -1057,7 +1062,38 @@ func (bsp *boundSetPredicate[T]) AsUnbound(r Reference, lits []Literal) UnboundP } func (bsp *boundSetPredicate[T]) Literals() Set[Literal] { - return bsp.lits + return bsp.literalView +} + +type readOnlyLiteralSet struct { + Set[Literal] +} + +func (readOnlyLiteralSet) Add(...Literal) { + panic("cannot add to read-only literal set") +} + +func (s readOnlyLiteralSet) Equals(other Set[Literal]) bool { Review Comment: This override makes `readOnlyLiteralSet.Equals(raw)` symmetric, but only from this side. The raw `literalSet.Equals` still asserts `other.(literalSet)` and returns false when it gets a `readOnlyLiteralSet`. So now that `Literals()` hands back the wrapper, any external caller doing `mySet.Equals(pred.Literals())` gets false even for identical contents, while the reverse direction is fine — an asymmetric-equality trap for anything using these as dedup/cache/equivalence keys. I'd mirror this unwrap in the raw `literalSet.Equals` too so both directions agree. wdyt? ########## bound_set_literal_view_test.go: ########## @@ -0,0 +1,100 @@ +// 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_test + +import ( + "slices" + "testing" + + iceberg "github.com/apache/iceberg-go" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestBoundSetPredicateLiteralViewClonesMutableMembers(t *testing.T) { + t.Parallel() + + geometry, err := iceberg.GeometryTypeOf("srid:4326") + require.NoError(t, err) + + tests := []struct { + name string + typ iceberg.Type + }{ + {name: "binary", typ: iceberg.PrimitiveTypes.Binary}, + {name: "fixed", typ: iceberg.FixedTypeOf(16)}, + {name: "geometry", typ: geometry}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + inputs := [][]byte{ + []byte("0123456789abcdef"), + []byte("fedcba9876543210"), + } + predicateLiterals := make([]iceberg.Literal, len(inputs)) + expectedLiterals := make([]iceberg.Literal, len(inputs)) + for i, input := range inputs { + expected, err := iceberg.LiteralFromBytes(tt.typ, slices.Clone(input)) + require.NoError(t, err) + expectedLiterals[i] = expected + + if tt.typ.Equals(geometry) { + predicateLiterals[i], err = iceberg.LiteralFromBytes(tt.typ, slices.Clone(input)) + require.NoError(t, err) + } else { + predicateLiterals[i] = iceberg.NewLiteral(slices.Clone(input)) + } + } + + predicate := iceberg.SetPredicate( + iceberg.OpIn, + iceberg.Reference("value"), + predicateLiterals, + ) + schema := iceberg.NewSchema(1, iceberg.NestedField{ID: 1, Name: "value", Type: tt.typ}) + + bound, err := predicate.(iceberg.UnboundPredicate).Bind(schema, true) + require.NoError(t, err) + literals := bound.(iceberg.BoundSetPredicate).Literals() + + for _, literal := range literals.Members() { + value, ok := literal.Any().([]byte) + require.True(t, ok) + value[0] ^= 0xff + } + for _, expected := range expectedLiterals { + assert.True(t, literals.Contains(expected)) + } + + assert.True(t, literals.All(func(literal iceberg.Literal) bool { + value, ok := literal.Any().([]byte) + require.True(t, ok) Review Comment: `require` inside the `All` callback calls `t.FailNow()` → `runtime.Goexit()`; if `All` ever dispatched the callback on another goroutine, that would exit the wrong goroutine and the failure wouldn't surface. `assert.True` is the safe call inside a callback. ########## bound_set_literal_view.go: ########## @@ -0,0 +1,49 @@ +// 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 + +func (s readOnlyLiteralSet) Members() []Literal { Review Comment: Tiny one — `readOnlyLiteralSet` is defined in `exprs.go` (struct, `Add`, `Equals`) but its `Members`/`All` and the `boundSetLiteralRef` mechanism live here, and the split doesn't track an obvious boundary. I'd consolidate the type plus `cloneBoundLiteral` into one file so it reads in one place. Non-blocking. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
