zeroshade commented on code in PR #2097:
URL: https://github.com/apache/iceberg-go/pull/2097#discussion_r4198578850
##########
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:
I'd keep `require.ErrorIs(t, err, iceberg.ErrType)` instead of going back to
`require.Error`. This is the only test that sends `VariantLiteral` members
through the bind-time set in `createBoundSetPredicate`, and with two distinct
values `ErrType` is only reachable if that set still holds both. If they
collapsed to one, binding takes the single-literal branch and fails with
`ErrInvalidArgument` (`ordered predicates are not supported on variant
fields`); I confirmed that with `IsIn(ref, one, build(int64(1)))`. Loosening
also doesn't help the test survive a future change: once variant set predicates
bind, `require.Error` fails just like `ErrorIs`. If you want to take the naming
point, something like `"bind to variant column keeps distinct members"` says
what the test checks.
<sub>Drafted with an AI-assisted tool; confirmed by a maintainer.</sub>
##########
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 length prefix does matter for correctness, in `Add`. `Contains` confirms
hits with `Equals`, but `addliteral` overwrites on a matching key without
checking. Boundary-shifted pairs can be built through the public constructors
because `variant.NewMetadata` accepts trailing bytes: `variant.New([]byte{0x01,
0x00, 0x00, 0x0C}, []byte{0x00})` and `variant.New([]byte{0x01, 0x00, 0x00},
[]byte{0x0C, 0x00})` are both valid and not `Equals`, yet their metadata and
value bytes concatenate to the same sequence. Without the prefix, adding the
second would silently drop the first; with it, the set keeps both. The comment
as written is accurate. Adding that `variantKey` must hash exactly what
`Equals` compares would be a good follow-up.
<sub>Drafted with an AI-assisted tool; confirmed by a maintainer.</sub>
##########
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:
`t.Helper()` alone won't change the attribution. The closures capture the
parent `t` and run inside subtests, so a failing `require` calls `FailNow` on
the parent from the subtest goroutine. The error is then logged under the
parent, and the subtest reports `subtest may have called FailNow on a parent
test`; I reproduced this with `t.Helper()` in place. Passing `t` in fixes it,
and there are already helpers that do that: `scalarVariant` in
`variant_cast_test.go` (package `iceberg`) and `variantLiteralOf` in
`literals_test.go` (package `iceberg_test`). Fine as a follow-up.
<sub>Drafted with an AI-assisted tool; confirmed by a maintainer.</sub>
--
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]
