This is an automated email from the ASF dual-hosted git repository.
laskoviymishka pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git
The following commit(s) were added to refs/heads/main by this push:
new ace9bf76f test: pin variant literal set edge cases (#2139)
ace9bf76f is described below
commit ace9bf76f613dd8fa4955eae4b56ebeb87c051bf
Author: Revanth <[email protected]>
AuthorDate: Wed Oct 7 06:54:50 2026 -0500
test: pin variant literal set edge cases (#2139)
---
exprs_test.go | 23 +++++++++-------------
literal_set_internal_test.go | 47 ++++++++++++++++++++++++++++++++------------
utils.go | 11 +++++++++--
3 files changed, 52 insertions(+), 29 deletions(-)
diff --git a/exprs_test.go b/exprs_test.go
index fbc314132..2ccd54a81 100644
--- a/exprs_test.go
+++ b/exprs_test.go
@@ -1132,20 +1132,12 @@ func TestVariantBoundLiteralRejectionMessage(t
*testing.T) {
}
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))
+ one := variant.Value(variantLiteralOf(t, int64(1)))
+ two := variant.Value(variantLiteralOf(t, int64(2)))
ref := iceberg.Reference("payload")
t.Run("in", func(t *testing.T) {
- pred := iceberg.IsIn(ref, one, two, build(int64(1)))
+ pred := iceberg.IsIn(ref, one, two,
variant.Value(variantLiteralOf(t, 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.
@@ -1159,13 +1151,16 @@ func TestVariantSetPredicate(t *testing.T) {
assert.True(t, pred.Negate().Equals(iceberg.IsIn(ref, one,
two)))
})
- t.Run("bind to variant column", func(t *testing.T) {
+ t.Run("bind to variant column keeps distinct members", 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.
+ // Set predicates on variant columns are not supported yet, so
binding
+ // falls through to the set-predicate ErrType. That error is
only
+ // reachable if the bind-time set still holds both members: had
they
+ // collapsed to one, binding would take the single-literal path
and
+ // fail with ErrInvalidArgument instead.
_, err := iceberg.BindExpr(sc, iceberg.IsIn(ref, one, two),
true)
require.ErrorIs(t, err, iceberg.ErrType)
})
diff --git a/literal_set_internal_test.go b/literal_set_internal_test.go
index 392c7d4ad..69c5955c6 100644
--- a/literal_set_internal_test.go
+++ b/literal_set_internal_test.go
@@ -18,6 +18,7 @@
package iceberg
import (
+ "slices"
"testing"
"github.com/apache/arrow-go/v18/parquet/variant"
@@ -26,27 +27,25 @@ import (
)
func TestLiteralSetVariant(t *testing.T) {
- build := func(v any) VariantLiteral {
- var b variant.Builder
- require.NoError(t, b.Append(v))
- val, err := b.Build()
- require.NoError(t, err)
+ // build takes the calling (sub)test's t so a failure is reported there.
+ build := func(t *testing.T, v any) VariantLiteral {
+ t.Helper()
- return VariantLiteral(val)
+ return VariantLiteral(scalarVariant(t, func(b *variant.Builder)
error { return b.Append(v) }))
}
t.Run("contains", func(t *testing.T) {
- set := newLiteralSet(build(int64(1)), build(int64(2)))
+ set := newLiteralSet(build(t, int64(1)), build(t, int64(2)))
require.Equal(t, 2, set.Len())
- assert.True(t, set.Contains(build(int64(1))))
- assert.True(t, set.Contains(build(int64(2))))
- assert.False(t, set.Contains(build(int64(3))))
+ assert.True(t, set.Contains(build(t, int64(1))))
+ assert.True(t, set.Contains(build(t, int64(2))))
+ assert.False(t, set.Contains(build(t, int64(3))))
})
t.Run("same value bytes with different metadata", func(t *testing.T) {
- objA := build(map[string]any{"a": int64(1)})
- objB := build(map[string]any{"b": int64(1)})
+ objA := build(t, map[string]any{"a": int64(1)})
+ objB := build(t, map[string]any{"b": int64(1)})
// Guard the premise: only the metadata dictionary tells these
apart.
require.Equal(t, variant.Value(objA).Bytes(),
variant.Value(objB).Bytes())
require.NotEqual(t, variant.Value(objA).Metadata().Bytes(),
variant.Value(objB).Metadata().Bytes())
@@ -55,7 +54,29 @@ func TestLiteralSetVariant(t *testing.T) {
assert.Equal(t, 2, set.Len())
set = newLiteralSet(objA)
- assert.True(t, set.Contains(build(map[string]any{"a":
int64(1)})))
+ assert.True(t, set.Contains(build(t, map[string]any{"a":
int64(1)})))
assert.False(t, set.Contains(objB))
})
+
+ t.Run("same concatenated bytes with shifted boundary", func(t
*testing.T) {
+ // Metadata accepts trailing bytes, so the metadata/value split
can
+ // move while the concatenation stays the same. addliteral
overwrites on
+ // a key match without calling Equals, so only the length
prefix in
+ // variantKey keeps both members.
+ left, err := variant.New([]byte{0x01, 0x00, 0x00, 0x0C},
[]byte{0x00})
+ require.NoError(t, err)
+ right, err := variant.New([]byte{0x01, 0x00, 0x00},
[]byte{0x0C, 0x00})
+ require.NoError(t, err)
+
+ // Guard the premise: distinct values with identical
concatenated bytes.
+ require.Equal(t,
+ slices.Concat(left.Metadata().Bytes(), left.Bytes()),
+ slices.Concat(right.Metadata().Bytes(), right.Bytes()))
+ require.False(t,
VariantLiteral(left).Equals(VariantLiteral(right)))
+
+ set := newLiteralSet(VariantLiteral(left),
VariantLiteral(right))
+ assert.Equal(t, 2, set.Len())
+ assert.True(t, set.Contains(VariantLiteral(left)))
+ assert.True(t, set.Contains(VariantLiteral(right)))
+ })
}
diff --git a/utils.go b/utils.go
index dbde6d6b9..d7ad97945 100644
--- a/utils.go
+++ b/utils.go
@@ -123,8 +123,15 @@ 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
-// the metadata ends and the value begins hash different input.
+// compares. It must hash exactly what Equals compares: if Equals changes (e.g.
+// to logical equality), this key has to change with it.
+//
+// The metadata is length-prefixed so values that differ only in where the
+// metadata ends and the value begins hash different input. This matters for
+// correctness, not just collision odds: addliteral overwrites on a key match
+// without calling Equals, and metadata may carry trailing bytes, so such pairs
+// are constructible. A genuine 64-bit hash collision still drops the earlier
+// member, the same trade-off as the Binary, Fixed, and Geo cases.
func variantKey(v VariantLiteral) uint64 {
val := variant.Value(v)
meta := val.Metadata().Bytes()
