laskoviymishka commented on code in PR #1872:
URL: https://github.com/apache/iceberg-go/pull/1872#discussion_r3844835279
##########
exprs_test.go:
##########
@@ -997,6 +997,31 @@ func TestBindAboveBelowIntMax(t *testing.T) {
assert.Equal(t, tt.exp, b)
})
}
+
+ t.Run("set predicates", func(t *testing.T) {
+ tests := []struct {
+ name string
+ pred iceberg.BooleanExpression
+ expected iceberg.BooleanExpression
+ }{
+ {"int in mixed range", iceberg.IsIn(ref, int64(34),
above, below), iceberg.EqualTo(ref, int32(34))},
+ {"int not in mixed range", iceberg.NotIn(ref,
int64(34), above, below), iceberg.NotEqualTo(ref, int32(34))},
+ {"int in outside range", iceberg.IsIn(ref, above,
below), iceberg.AlwaysFalse{}},
+ {"int not in outside range", iceberg.NotIn(ref, above,
below), iceberg.AlwaysTrue{}},
+ {"float in mixed range", iceberg.IsIn(ref2,
float64(34), above2, below2), iceberg.EqualTo(ref2, float32(34))},
+ {"float not in outside range", iceberg.NotIn(ref2,
above2, below2), iceberg.AlwaysTrue{}},
Review Comment:
The int side has four cases but the float side only two, with no structural
reason for the gap. The float path goes through `Float32AboveMaxLiteral` vs
int's `Int32AboveMaxLiteral`, so the missing cases aren't redundant. I'd fill
in the two symmetric ones:
```go
{"float in outside range", iceberg.IsIn(ref2, above2, below2),
iceberg.AlwaysFalse{}},
{"float not in mixed range", iceberg.NotIn(ref2, float64(34), above2,
below2), iceberg.NotEqualTo(ref2, float32(34))},
```
While we're here, every case in this table leaves 0 or 1 survivor, so the 2+
survivor branch never runs with sentinels present. One case like `IsIn(ref,
int64(34), int64(35), above, below)` -> `IsIn(ref, int32(34), int32(35))` would
exercise it.
##########
table/evaluators_test.go:
##########
@@ -1929,6 +1929,7 @@ func (suite *InclusiveMetricsTestSuite) TestInMetrics() {
{iceberg.IsIn(ref, IntMinValue-1, IntMinValue), true, "should
read: id equal to lower bound"},
{iceberg.IsIn(ref, IntMaxValue-4, IntMaxValue-3), true, "should
read: id between upper and lower bounds"},
{iceberg.IsIn(ref, IntMaxValue, IntMaxValue+1), true, "should
read: id equal to upper bound"},
+ {iceberg.IsIn(ref, int64(IntMaxValue), int64(math.MaxInt32)+1),
true, "should read: ignore value outside int32 range"},
Review Comment:
This regression only exercises `IsIn`, but the original panic hit `NotIn`
too on the same path (an `AboveMaxLiteral` reaching `cloneBoundLiteral`). The
fix is in the shared `createBoundSetPredicate` so both are covered, but this is
the metrics evaluator where it actually blew up, so I'd add the NOT IN sibling
here to pin the end-to-end path:
```go
{iceberg.NotIn(ref, int64(IntMaxValue), int64(math.MaxInt32)+1), true,
"should read: ignore value outside int32 range in not-in"},
```
##########
exprs_test.go:
##########
@@ -997,6 +997,31 @@ func TestBindAboveBelowIntMax(t *testing.T) {
assert.Equal(t, tt.exp, b)
})
}
+
+ t.Run("set predicates", func(t *testing.T) {
+ tests := []struct {
+ name string
+ pred iceberg.BooleanExpression
+ expected iceberg.BooleanExpression
+ }{
+ {"int in mixed range", iceberg.IsIn(ref, int64(34),
above, below), iceberg.EqualTo(ref, int32(34))},
+ {"int not in mixed range", iceberg.NotIn(ref,
int64(34), above, below), iceberg.NotEqualTo(ref, int32(34))},
+ {"int in outside range", iceberg.IsIn(ref, above,
below), iceberg.AlwaysFalse{}},
+ {"int not in outside range", iceberg.NotIn(ref, above,
below), iceberg.AlwaysTrue{}},
+ {"float in mixed range", iceberg.IsIn(ref2,
float64(34), above2, below2), iceberg.EqualTo(ref2, float32(34))},
+ {"float not in outside range", iceberg.NotIn(ref2,
above2, below2), iceberg.AlwaysTrue{}},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ bound, err := iceberg.BindExpr(sc, tt.pred,
true)
+ require.NoError(t, err)
+ expected, err := iceberg.BindExpr(sc,
tt.expected, true)
Review Comment:
`expected` here is the bound form while `tt.expected` is the unbound input,
so the name's doing double duty. I'd rename the local to `wantBound` to pair
with `bound` above and make the `Equals` read cleaner. 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]
