laskoviymishka commented on code in PR #2117:
URL: https://github.com/apache/iceberg-go/pull/2117#discussion_r4193441068
##########
table/in_predicate_extrema_test.go:
##########
@@ -139,6 +140,107 @@ func TestManifestEvaluatorInPredicateExtrema(t
*testing.T) {
}
}
+func TestInclusiveMetricsEvaluatorInPredicateExtrema(t *testing.T) {
+ encode := func(value int32) []byte {
+ encoded, err := iceberg.NewLiteral(value).MarshalBinary()
+ require.NoError(t, err)
+
+ return encoded
+ }
+
+ schema := iceberg.NewSchema(1, iceberg.NestedField{ID: 1, Name:
"value", Type: iceberg.PrimitiveTypes.Int32})
+ expr := iceberg.IsIn(iceberg.Reference("value"), int32(1), int32(100))
Review Comment:
The set here is int32-only, so nothing exercises `getCmpLiteral(minLit)` on
the types where it matters: string, decimal, binary, date/timestamp, and
especially the variant `BoundExtract` and geo literals where `getCmpLiteral`
panics on an unrecognized type. I'd add a table-driven case running a few of
those through the fast path.
Separately, none of these cases prove the extrema path is even taken, since
every one returns the same answer through the slow `removeBoundCheck`. A
regression in `boundSetExtremaRef` or the dispatcher would leave the whole
suite green. I'd either call `VisitInWithExtrema` directly with explicit
min/max in at least one case, or assert both entry points agree across the
matrix the way the manifest test does.
##########
table/evaluators.go:
##########
@@ -1153,8 +1165,38 @@ func (m *inclusiveMetricsEval) VisitIn(t
iceberg.BoundTerm, s iceberg.Set[iceber
return rowsMightMatch
}
- values := s.Members()
- if lowerBound, ok := m.boundFor(t, m.lowerBounds[fieldID]); ok {
+ hasExtrema := minLit != nil && maxLit != nil
+ var (
+ values []iceberg.Literal
+ lowerBound, upperBound iceberg.Literal
+ hasLowerBound bool
+ hasUpperBound bool
+ )
+ if hasExtrema {
Review Comment:
I'd fetch both bounds once up top and gate the extrema prune in a single `if
hasExtrema` block, then fall through to the unchanged `Members()` and
`removeBoundCheck` tail. Right now the lower-bound NaN check and the `boundFor`
fetch are duplicated across the `hasExtrema` and `else` branches, and
`hasExtrema` gets tested a second time further down to decide whether to
re-fetch the upper bound, so any future change to bound handling has to land in
three places and the two paths are easy to drift apart.
One thing to preserve: the current order means a lower-bound prune returns
before the upper `boundFor` ever runs, and the `disjoint lower bound avoids
malformed upper bound` test leans on exactly that. If we hoist the upper fetch
to the top, that test's malformed `[]byte{1}` upper would now get decoded. So
either keep the extrema prune ordered lower-then-upper so the short-circuit
still skips the upper fetch, or pull the shared tail into a small
`prunedByExtrema(t, fieldID, minLit, maxLit) bool` helper that leaves the
existing body untouched. Either's fine, but I'd rather not have the fast path
spliced into the middle of the old flow.
##########
table/in_predicate_extrema_bench_test.go:
##########
@@ -116,3 +119,93 @@ func benchmarkManifestIn[T iceberg.LiteralType](b
*testing.B, values []T, lower,
manifestInPredicateBenchmarkSink = matched
}
}
+
+func BenchmarkInclusiveMetricsEvalInPredicate(b *testing.B) {
+ for _, literalCount := range []int{10, 50, 200} {
+ values := make([]int32, literalCount)
+ for i := range values {
+ values[i] = int32(i * 2)
+ }
+
+ maxValue := values[len(values)-1]
+ for _, bounds := range []struct {
+ name string
+ lower int32
+ upper int32
+ }{
+ {name: "below-set", lower: -2, upper: -2},
+ {name: "above-set", lower: maxValue + 2, upper:
maxValue + 2},
+ {name: "overlap", lower: values[0], upper: maxValue},
+ {name: "sparse-gap", lower: values[literalCount/2] + 1,
upper: values[literalCount/2] + 1},
Review Comment:
The overlap and sparse-gap cases both land inside the set range, so they go
through the slow `removeBoundCheck` and can't show any fast-path gain. Only
below-set and above-set do. Worth a note here (or in the PR description) that
these two are regression baselines rather than fast-path cases, so the numbers
don't get miscredited.
##########
table/in_predicate_extrema_test.go:
##########
@@ -139,6 +140,107 @@ func TestManifestEvaluatorInPredicateExtrema(t
*testing.T) {
}
}
+func TestInclusiveMetricsEvaluatorInPredicateExtrema(t *testing.T) {
+ encode := func(value int32) []byte {
+ encoded, err := iceberg.NewLiteral(value).MarshalBinary()
+ require.NoError(t, err)
+
+ return encoded
+ }
+
+ schema := iceberg.NewSchema(1, iceberg.NestedField{ID: 1, Name:
"value", Type: iceberg.PrimitiveTypes.Int32})
+ expr := iceberg.IsIn(iceberg.Reference("value"), int32(1), int32(100))
+ eval, err := newInclusiveMetricsEvaluator(schema, expr, true, true)
+ require.NoError(t, err)
+
+ tests := []struct {
+ name string
+ lowerBound []byte
+ upperBound []byte
+ want bool
+ }{
+ {name: "lower bound above set", lowerBound: encode(101),
upperBound: encode(200), want: false},
+ {name: "upper bound below set", lowerBound: encode(-10),
upperBound: encode(0), want: false},
+ {name: "sparse set has no member in range", lowerBound:
encode(40), upperBound: encode(60), want: false},
+ {name: "overlapping range contains member", lowerBound:
encode(50), upperBound: encode(100), want: true},
+ {name: "upper bound equals minimum literal", upperBound:
encode(1), want: true},
+ {name: "lower bound equals maximum literal", lowerBound:
encode(100), want: true},
+ {name: "missing bounds", want: true},
+ {name: "upper bound below set without lower bound", upperBound:
encode(0), want: false},
+ {name: "lower bound above set without upper bound", lowerBound:
encode(101), want: false},
+ {name: "disjoint lower bound avoids malformed upper bound",
lowerBound: encode(101), upperBound: []byte{1}, want: false},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ file := inclusiveMetricsInTestFile(t, tt.lowerBound,
tt.upperBound)
+ got, err := eval(file)
+ require.NoError(t, err)
+ assert.Equal(t, tt.want, got)
+ })
+ }
+
+ t.Run("NaN bounds fail open", func(t *testing.T) {
+ nan, err := iceberg.NewLiteral(math.NaN()).MarshalBinary()
+ require.NoError(t, err)
+ floatSchema := iceberg.NewSchema(1, iceberg.NestedField{
+ ID: 1, Name: "value", Type:
iceberg.PrimitiveTypes.Float64,
+ })
+ floatEval, err := newInclusiveMetricsEvaluator(
+ floatSchema, iceberg.IsIn(iceberg.Reference("value"),
float64(1), float64(2)), true, true,
+ )
+ require.NoError(t, err)
+
+ got, err := floatEval(inclusiveMetricsInTestFile(t, nan, nan))
+ require.NoError(t, err)
+ assert.True(t, got)
+ })
+
+ t.Run("oversized set keeps existing fallback", func(t *testing.T) {
+ values := make([]int32, inPredicateLimit+1)
+ for i := range values {
+ values[i] = int32(i)
+ }
+ largeEval, err := newInclusiveMetricsEvaluator(
+ schema, iceberg.IsIn(iceberg.Reference("value"),
values...), true, true,
+ )
+ require.NoError(t, err)
+
+ got, err := largeEval(inclusiveMetricsInTestFile(
+ t, encode(int32(inPredicateLimit+1)),
encode(int32(inPredicateLimit+1)),
Review Comment:
This case is meant to prove the oversized-set fallback fires before the
extrema prune, but as written it doesn't isolate that. I'd pick bounds clearly
disjoint from the set's range so that if the `Len() > inPredicateLimit` guard
regressed, the fast path would prune and flip the expected answer to false.
As-is a missing guard could still pass.
##########
table/in_predicate_extrema_bench_test.go:
##########
@@ -116,3 +119,93 @@ func benchmarkManifestIn[T iceberg.LiteralType](b
*testing.B, values []T, lower,
manifestInPredicateBenchmarkSink = matched
}
}
+
+func BenchmarkInclusiveMetricsEvalInPredicate(b *testing.B) {
+ for _, literalCount := range []int{10, 50, 200} {
+ values := make([]int32, literalCount)
+ for i := range values {
+ values[i] = int32(i * 2)
+ }
+
+ maxValue := values[len(values)-1]
+ for _, bounds := range []struct {
+ name string
+ lower int32
+ upper int32
+ }{
+ {name: "below-set", lower: -2, upper: -2},
+ {name: "above-set", lower: maxValue + 2, upper:
maxValue + 2},
+ {name: "overlap", lower: values[0], upper: maxValue},
+ {name: "sparse-gap", lower: values[literalCount/2] + 1,
upper: values[literalCount/2] + 1},
+ } {
+
b.Run("int32/in="+strconv.Itoa(literalCount)+"/"+bounds.name, func(b
*testing.B) {
+ benchmarkInclusiveMetricsIn(b, values,
bounds.lower, bounds.upper, literalCount, bounds.name == "overlap")
+ })
+ }
+ }
+}
+
+func benchmarkInclusiveMetricsIn(b *testing.B, values []int32, lower, upper
int32, literalCount int, wantMatch bool) {
+ b.Helper()
+
+ lowerBytes, err := iceberg.NewLiteral(lower).MarshalBinary()
+ if err != nil {
+ b.Fatal(err)
+ }
+ upperBytes, err := iceberg.NewLiteral(upper).MarshalBinary()
+ if err != nil {
+ b.Fatal(err)
+ }
+
+ schema := iceberg.NewSchema(1, iceberg.NestedField{ID: 1, Name:
"value", Type: iceberg.PrimitiveTypes.Int32})
+ eval, err := newInclusiveMetricsEvaluator(
+ schema, iceberg.IsIn(iceberg.Reference("value"), values...),
true, true,
+ )
+ if err != nil {
+ b.Fatal(err)
+ }
+
+ builder, err := iceberg.NewDataFileBuilder(
+ *iceberg.UnpartitionedSpec, iceberg.EntryContentData,
"file.parquet", iceberg.ParquetFile,
+ nil, nil, nil, 10, 100,
+ )
+ if err != nil {
+ b.Fatal(err)
+ }
+ file := builder.
+ ValueCounts(map[int]int64{1: 10}).
+ NullValueCounts(map[int]int64{1: 0}).
+ NaNValueCounts(map[int]int64{1: 0}).
+ LowerBoundValues(map[int][]byte{1: lowerBytes}).
+ UpperBoundValues(map[int][]byte{1: upperBytes}).
+ Build()
+ files := make([]iceberg.DataFile, 10_000)
+ for i := range files {
+ files[i] = file
+ }
+
+ b.ReportAllocs()
+ b.ReportMetric(float64(len(files)), "files/op")
+ b.ReportMetric(float64(literalCount), "literals/file")
+ b.ResetTimer()
+ for range b.N {
+ matchedFiles := 0
+ for _, file := range files {
Review Comment:
Couple of small things in the bench: the inner `for _, file := range files`
shadows the outer `file`, which is legal but confusing, so I'd rename it to
`f`. And `wantMatch` is derived from `bounds.name == "overlap"` at the call
site; I'd put a `wantMatch bool` field on the bounds case struct instead so the
expected result lives with the data.
##########
table/evaluators.go:
##########
@@ -1142,6 +1142,18 @@ func (m *inclusiveMetricsEval)
VisitNotEqual(iceberg.BoundTerm, iceberg.Literal)
}
func (m *inclusiveMetricsEval) VisitIn(t iceberg.BoundTerm, s
iceberg.Set[iceberg.Literal]) bool {
+ return m.visitIn(t, s, nil, nil)
+}
+
+func (m *inclusiveMetricsEval) VisitInWithExtrema(
Review Comment:
Worth a one-line doc comment spelling out the contract: both `minLit` and
`maxLit` must be non-nil (and the actual min/max of `s`), or both nil to get
the slow path. Right now a partial-nil call silently falls back, and
`manifestEvalVisitor.visitIn` treats the two independently, so the two extrema
implementations have different partial-nil semantics. I'd align them or note
the difference in the comment.
##########
table/in_predicate_extrema_test.go:
##########
@@ -139,6 +140,107 @@ func TestManifestEvaluatorInPredicateExtrema(t
*testing.T) {
}
}
+func TestInclusiveMetricsEvaluatorInPredicateExtrema(t *testing.T) {
+ encode := func(value int32) []byte {
+ encoded, err := iceberg.NewLiteral(value).MarshalBinary()
+ require.NoError(t, err)
+
+ return encoded
+ }
+
+ schema := iceberg.NewSchema(1, iceberg.NestedField{ID: 1, Name:
"value", Type: iceberg.PrimitiveTypes.Int32})
+ expr := iceberg.IsIn(iceberg.Reference("value"), int32(1), int32(100))
+ eval, err := newInclusiveMetricsEvaluator(schema, expr, true, true)
+ require.NoError(t, err)
+
+ tests := []struct {
+ name string
+ lowerBound []byte
+ upperBound []byte
+ want bool
+ }{
+ {name: "lower bound above set", lowerBound: encode(101),
upperBound: encode(200), want: false},
+ {name: "upper bound below set", lowerBound: encode(-10),
upperBound: encode(0), want: false},
+ {name: "sparse set has no member in range", lowerBound:
encode(40), upperBound: encode(60), want: false},
+ {name: "overlapping range contains member", lowerBound:
encode(50), upperBound: encode(100), want: true},
+ {name: "upper bound equals minimum literal", upperBound:
encode(1), want: true},
+ {name: "lower bound equals maximum literal", lowerBound:
encode(100), want: true},
+ {name: "missing bounds", want: true},
+ {name: "upper bound below set without lower bound", upperBound:
encode(0), want: false},
+ {name: "lower bound above set without upper bound", lowerBound:
encode(101), want: false},
+ {name: "disjoint lower bound avoids malformed upper bound",
lowerBound: encode(101), upperBound: []byte{1}, want: false},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ file := inclusiveMetricsInTestFile(t, tt.lowerBound,
tt.upperBound)
+ got, err := eval(file)
+ require.NoError(t, err)
+ assert.Equal(t, tt.want, got)
+ })
+ }
+
+ t.Run("NaN bounds fail open", func(t *testing.T) {
Review Comment:
The NaN coverage only hits the both-bounds-NaN case. The
`!m.isNan(upperBound)` skip in the extrema branch (a NaN upper with a finite
lower) isn't exercised by anything, so I'd add that case plus the mirror
finite-upper/NaN-lower so both fail-open branches are pinned.
--
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]
