You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "mattfaltyn (via GitHub)" <gi...@apache.org> on 2026/08/23 16:29:15 UTC
[PR] fix(expr): drop out-of-range set literals [iceberg-go]
mattfaltyn opened a new pull request, #1872:
URL: https://github.com/apache/iceberg-go/pull/1872
## Summary
- discard above-max and below-min sentinels while binding `IN` and `NOT IN` predicates
- reuse the existing empty- and singleton-set simplifications after impossible values are removed
- cover integer and floating-point narrowing plus the inclusive metrics scan-planning regression
## Why
Applications can construct a filter with a wider Go literal type than the Iceberg field type. When a set mixes representable and out-of-range values, binding currently retains range sentinels that cannot equal any field value. Metrics pruning later treats those sentinels as regular typed literals and returns an interface-conversion error instead of planning the scan.
Removing impossible set members during binding preserves membership semantics and keeps sentinels out of downstream predicate consumers.
Closes #1870.
## Testing
- `go test . -run '^TestBindAboveBelowIntMax$' -count=1`
- `go test ./table -run '^TestEvaluators/TestInMetrics$' -count=1 -v`
- `go test ./...`
- `make test-assert`
- `make test-race`
- `golangci-lint v2.12.2 run --timeout=10m --new-from-rev origin/main` (`0 issues`)
The full repository lint invocation also reports two existing `intrange` findings in unchanged `catalog/rest/load_table_bench_test.go` lines 84 and 101.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(expr): drop out-of-range set literals [iceberg-go]
Posted by "mattfaltyn (via GitHub)" <gi...@apache.org>.
mattfaltyn commented on code in PR #1872:
URL: https://github.com/apache/iceberg-go/pull/1872#discussion_r3850768034
##########
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:
Thanks — added the two symmetric float cases and a multi-survivor IN case in 1b28a8d, so the filtered bound-set branch is covered too.
##########
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:
Thanks — renamed the bound expectation to wantBound in 1b28a8d.
##########
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:
Thanks — added the NOT IN regression in 1b28a8d under strict metrics, where NOT IN inspects the set and reproduces the pre-fix sentinel failure. It keeps two in-range survivors so the fixed path remains a bound NOT IN set.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(expr): drop out-of-range set literals [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(expr): drop out-of-range set literals [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1872:
URL: https://github.com/apache/iceberg-go/pull/1872
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(expr): drop out-of-range set literals [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade closed pull request #1872: fix(expr): drop out-of-range set literals
URL: https://github.com/apache/iceberg-go/pull/1872
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(expr): drop out-of-range set literals [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on PR #1872:
URL: https://github.com/apache/iceberg-go/pull/1872#issuecomment-5408973724
I think this need rebase
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org