laskoviymishka commented on code in PR #1911:
URL: https://github.com/apache/iceberg-go/pull/1911#discussion_r3874737623
##########
exprs.go:
##########
@@ -1008,14 +1008,64 @@ func createBoundSetPredicate(op Operation, term
BoundTerm, lits Set[Literal]) (B
ErrType, term.Type())
}
-func newBoundSetPredicate[T LiteralType](op Operation, term BoundTerm, lits
Set[Literal]) *boundSetPredicate[T] {
- return &boundSetPredicate[T]{op: op, term: term, lits: lits}
+func newBoundSetPredicate[T LiteralType](op Operation, term BoundTerm, lits
literalSet) *boundSetPredicate[T] {
+ predicate := &boundSetPredicate[T]{op: op, term: term, lits: lits}
+ if op == OpIn {
+ predicate.minLiteral, predicate.maxLiteral,
predicate.hasExtrema = literalSetExtrema[T](lits)
+ }
+
+ return predicate
}
type boundSetPredicate[T LiteralType] struct {
- op Operation
- term BoundTerm
- lits Set[Literal]
+ op Operation
+ term BoundTerm
+ lits Set[Literal]
+ minLiteral Literal
+ maxLiteral Literal
+ hasExtrema bool
+}
+
+func literalSetExtrema[T LiteralType](lits literalSet) (min, max Literal, ok
bool) {
Review Comment:
`min` and `max` have been builtins since Go 1.21 and the module's on 1.25,
so these named returns shadow them — `golangci-lint`'s `predeclared` will flag
it (same for the `boundSetExtremaRef` signature in `bound_set_literal_view.go`
and the destructuring in `visitors.go`). I'd rename to `minLit`/`maxLit` to
match the `minLiteral`/`maxLiteral` struct fields, consistently across all
three sites.
##########
exprs.go:
##########
@@ -1031,8 +1081,12 @@ func (bsp *boundSetPredicate[T]) Equals(other
BooleanExpression) bool {
func (bsp *boundSetPredicate[T]) Op() Operation { return bsp.op }
func (bsp *boundSetPredicate[T]) Negate() BooleanExpression {
return &boundSetPredicate[T]{
- op: bsp.op.Negate(), term: bsp.term,
- lits: bsp.lits,
+ op: bsp.op.Negate(),
+ term: bsp.term,
Review Comment:
These extrema get carried onto the `NOT IN` result, but the dispatch in
`visitBoundPredicate` only consults them on `OpIn`, so for `OpNotIn` they're
stored and never read — 24 bytes per negated predicate for nothing. I'd set
`hasExtrema: false` here. (Minor gotcha the other way: a double-negate back to
`OpIn` loses them, since `Negate` doesn't recompute.)
##########
table/evaluators.go:
##########
@@ -162,7 +164,53 @@ func allBoundCheck(bound iceberg.Literal, set
iceberg.Set[iceberg.Literal], want
panic(iceberg.ErrType)
}
+func compareBound[T iceberg.LiteralType](bound, value iceberg.Literal) int {
+ val := bound.(iceberg.TypedLiteral[T])
+
+ return val.Comparator()(val.Value(),
value.(iceberg.TypedLiteral[T]).Value())
+}
+
+func boundCompare(bound, value iceberg.Literal) int {
Review Comment:
`boundCompare` is a second copy of the type dispatch `getCmpLiteral` already
does — `boundCompare(a, b)` is just `getCmpLiteral(a)(a, b)`. The only reason
it exists is that `getCmpLiteral` is missing the `TimestampNano` case, so
rather than adding it there the PR routes around it with a parallel switch.
The catch is that leaves the root gap in place: `getCmpLiteral` still panics
on nanosecond-timestamp columns, so `VisitEqual`/`VisitGreater`/etc. still blow
up for a TimestampNs partition column even though the IN path now handles it.
`removeBoundCheck` has the same hole for the inclusive/strict metrics IN paths.
So we've added TimestampNs coverage to three dispatchers but not the two that
share the same shape.
I'd add the `TimestampNano` case to `getCmpLiteral` (and
`removeBoundCheck`), then collapse `boundCompare` to
`getCmpLiteral(bound)(bound, value)` and drop `compareBound` — that finishes
the TimestampNano fix everywhere and removes ~28 lines of parallel dispatch.
wdyt?
##########
table/evaluators.go:
##########
@@ -179,7 +227,11 @@ func (m *manifestEvalVisitor) VisitIn(term
iceberg.BoundTerm, literals iceberg.S
panic(err)
}
- if allBoundCheck(lower, literals, 1) {
+ if max != nil {
Review Comment:
I think the extrema check never runs for the sets this is meant to speed up.
The `literals.Len() > inPredicateLimit` guard above still fires first and
returns `rowsMightMatch`, so any IN set past 200 values short-circuits before
we reach `if max != nil` — and extrema get computed at bind time for every set
size regardless. The O(1) win is exactly inert for the large predicates that
motivated the change.
That guard only makes sense for the O(n) `allBoundCheck` fallback. I'd move
it into the `else` arm so the extrema comparison runs unconditionally, and we
only fall through to the length guard + `allBoundCheck` when extrema aren't
available (`max == nil`).
The benchmark tops out at exactly 200 literals, right at the cutoff, so this
regime never gets exercised — a 201+ case with a disjoint partition range would
return `rowsMightMatch` where we'd want `rowsCannotMatch`. wdyt?
##########
table/in_predicate_extrema_test.go:
##########
@@ -0,0 +1,114 @@
+// 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 table
+
+import (
+ "testing"
+
+ "github.com/apache/arrow-go/v18/arrow/decimal128"
+ "github.com/apache/iceberg-go"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+func TestManifestEvaluatorInPredicateExtrema(t *testing.T) {
+ decimal := func(value int64) iceberg.Decimal {
+ return iceberg.Decimal{Val: decimal128.FromI64(value), Scale: 2}
+ }
+
+ tests := []struct {
+ name string
+ typ iceberg.Type
+ expr iceberg.BooleanExpression
+ lower iceberg.Literal
+ upper iceberg.Literal
+ expectRead bool
+ }{
+ {
+ name: "decimal below lower bound",
+ typ: iceberg.DecimalTypeOf(12, 2),
+ expr: iceberg.IsIn(iceberg.Reference("value"),
decimal(100), decimal(200), decimal(300)),
+ lower: iceberg.NewLiteral(decimal(400)),
+ upper: iceberg.NewLiteral(decimal(500)),
+ expectRead: false,
+ },
+ {
+ name: "decimal above upper bound",
+ typ: iceberg.DecimalTypeOf(12, 2),
+ expr: iceberg.IsIn(iceberg.Reference("value"),
decimal(100), decimal(200), decimal(300)),
+ lower: iceberg.NewLiteral(decimal(-100)),
+ upper: iceberg.NewLiteral(decimal(0)),
+ expectRead: false,
+ },
+ {
+ name: "decimal overlaps bound",
+ typ: iceberg.DecimalTypeOf(12, 2),
+ expr: iceberg.IsIn(iceberg.Reference("value"),
decimal(100), decimal(200), decimal(300)),
+ lower: iceberg.NewLiteral(decimal(200)),
+ upper: iceberg.NewLiteral(decimal(250)),
+ expectRead: true,
+ },
+ {
+ name: "timestamp nanos below lower bound",
+ typ: iceberg.PrimitiveTypes.TimestampNs,
+ expr: iceberg.IsIn(iceberg.Reference("value"),
iceberg.TimestampNano(100), iceberg.TimestampNano(200),
iceberg.TimestampNano(300)),
+ lower:
iceberg.NewLiteral(iceberg.TimestampNano(400)),
+ upper:
iceberg.NewLiteral(iceberg.TimestampNano(500)),
+ expectRead: false,
+ },
+ {
+ name: "timestamp nanos above upper bound",
+ typ: iceberg.PrimitiveTypes.TimestampNs,
+ expr: iceberg.IsIn(iceberg.Reference("value"),
iceberg.TimestampNano(100), iceberg.TimestampNano(200),
iceberg.TimestampNano(300)),
+ lower:
iceberg.NewLiteral(iceberg.TimestampNano(-100)),
+ upper:
iceberg.NewLiteral(iceberg.TimestampNano(99)),
+ expectRead: false,
+ },
+ {
+ name: "timestamp nanos overlaps bound",
+ typ: iceberg.PrimitiveTypes.TimestampNs,
+ expr: iceberg.IsIn(iceberg.Reference("value"),
iceberg.TimestampNano(100), iceberg.TimestampNano(200),
iceberg.TimestampNano(300)),
+ lower:
iceberg.NewLiteral(iceberg.TimestampNano(200)),
+ upper:
iceberg.NewLiteral(iceberg.TimestampNano(250)),
+ expectRead: true,
+ },
Review Comment:
Could we add a case with 201+ literals and a clearly disjoint partition
range here? That's the one that would catch the `inPredicateLimit` ordering
issue — today it returns `rowsMightMatch` (read) where the extrema say
`rowsCannotMatch` (prune). A boundary case where `lower == max` (expecting a
read) would lock the `== 1` edge too.
##########
table/evaluators.go:
##########
@@ -179,7 +227,11 @@ func (m *manifestEvalVisitor) VisitIn(term
iceberg.BoundTerm, literals iceberg.S
panic(err)
}
- if allBoundCheck(lower, literals, 1) {
+ if max != nil {
+ if boundCompare(lower, max) == 1 {
Review Comment:
The `Comparator` contract in `literals.go` is defined as `< 0` / `> 0`, not
`±1`, so checking `== 1` here (and `== -1` on the upper bound) is a little
fragile — a conforming comparator that returns 2 would silently skip the prune
and we'd read the manifest anyway. I'd use `boundCompare(lower, max) > 0` and
`boundCompare(upper, min) < 0`.
There are pre-existing `== 1`/`== -1` sites elsewhere in this file so this
isn't new, but since we're adding two more I'd rather not extend the pattern.
##########
visitors.go:
##########
@@ -82,6 +82,10 @@ type BoundGeospatialExprVisitor[T any] interface {
VisitBBoxNotIntersects(BoundTerm, BoundingBox) T
}
+type boundSetExtremaExprVisitor[T any] interface {
Review Comment:
Worth a doc comment here: `VisitInWithExtrema` only gets called on the
`borrowed` path in `visitBoundPredicate`, so a visitor that implements it but
is driven through the public `VisitBoundPredicate` will silently never see it.
The geospatial extension interface right above documents its dispatch contract
explicitly; I'd mirror that.
Related, only `manifestEvalVisitor` implements it today — the other three
evaluators now go through `VisitBoundPredicateRef` and fall back to `VisitIn`,
so inclusive/strict metrics and row-group filtering still scan every literal.
Fine to leave for a follow-up, but a note here keeps it from reading as if all
four are optimized.
--
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]
