laskoviymishka commented on code in PR #2097:
URL: https://github.com/apache/iceberg-go/pull/2097#discussion_r4193144911
##########
utils.go:
##########
@@ -119,6 +122,23 @@ var lzseed = maphash.MakeSeed()
type literalSet map[any]struct{ orig Literal }
+// variantKey hashes the metadata and value bytes that VariantLiteral.Equals
+// compares. The metadata is length-prefixed so values that differ only in
where
Review Comment:
The keying's correct — this just reads like the length-prefix is
load-bearing for correctness, and it isn't. `Contains` always confirms a hit
with `lit.Equals(v.orig)`, and `Equals` compares metadata and value as separate
slices, so a shifted-boundary collision can't produce a false positive on
lookup; the prefix only lowers collision odds. Worth rewording.
While you're in here, it's worth saying out loud that an add-time hash
collision silently overwrites the earlier member (`l[variantKey(v)] = ...`) —
the same ~2^-64 we already accept for Binary/Fixed/Geo, fine to live with but
not obvious from the code — and that `variantKey` has to move in lockstep with
`Equals`: if `Equals` ever becomes logical equality, this hash has to follow.
All follow-up, nothing blocking.
##########
exprs_test.go:
##########
@@ -1131,6 +1131,60 @@ func TestVariantBoundLiteralRejectionMessage(t
*testing.T) {
assert.ErrorContains(t, err, "ordered predicates are not supported on
variant fields")
}
+func TestVariantSetPredicate(t *testing.T) {
+ build := func(v any) variant.Value {
+ var b variant.Builder
+ require.NoError(t, b.Append(v))
+ val, err := b.Build()
+ require.NoError(t, err)
+
+ return val
+ }
+
+ one, two := build(int64(1)), build(int64(2))
+ ref := iceberg.Reference("payload")
+
+ t.Run("in", func(t *testing.T) {
+ pred := iceberg.IsIn(ref, one, two, build(int64(1)))
+ require.Implements(t, (*iceberg.UnboundPredicate)(nil), pred)
+ assert.Equal(t, iceberg.OpIn, pred.Op())
+ // The duplicate is a separate buffer, so this only holds if
the set dedups by content.
+ assert.True(t, pred.Equals(iceberg.IsIn(ref, one, two)))
+ })
+
+ t.Run("not in", func(t *testing.T) {
+ pred := iceberg.NotIn(ref, one, two)
+ require.Implements(t, (*iceberg.UnboundPredicate)(nil), pred)
+ assert.Equal(t, iceberg.OpNotIn, pred.Op())
+ assert.True(t, pred.Negate().Equals(iceberg.IsIn(ref, one,
two)))
+ })
+
+ t.Run("bind to variant column", func(t *testing.T) {
+ sc := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "payload", Type:
iceberg.VariantType{}, Required: false},
+ )
+
+ // Set predicates on variant columns are not supported yet;
binding
+ // falls through to the set-predicate type error.
+ _, err := iceberg.BindExpr(sc, iceberg.IsIn(ref, one, two),
true)
+ require.ErrorIs(t, err, iceberg.ErrType)
Review Comment:
Follow-up, not blocking: this pins "variant columns reject at bind with
`ErrType`" as if it were intended contract, but the comment says it's just
not-supported-yet — when variant set support lands this'll fail for a reason
unrelated to the change that breaks it. If the point is only that binding
doesn't panic, I'd rename it to say that and loosen to `require.Error` so it
survives the feature landing. (+1 to zeroshade's flag on this subtest.)
##########
literal_set_internal_test.go:
##########
@@ -0,0 +1,61 @@
+// 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 (
+ "testing"
+
+ "github.com/apache/arrow-go/v18/parquet/variant"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+func TestLiteralSetVariant(t *testing.T) {
+ build := func(v any) VariantLiteral {
Review Comment:
Nit for a follow-up: `build` calls `require` on the outer `t`, so a failure
inside the closure gets attributed to the parent test rather than the subtest
that triggered it. A `t.Helper()` at the top of the closure fixes the
attribution. Same closure shows up in `exprs_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: [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]
