perf(table): precompute IN predicate extrema

[fallintoplace]

What changed

• Precompute min/max for IN when the bound expression is created.
• Use one comparison per manifest instead of scanning every literal for both partition bounds.
• Keep existing visitor behavior for public visitors and literals without a total ordering.
• Added focused tests and a benchmark for int32, string, decimal, binary, and nanosecond timestamp values.

Benchmark

• Machine: Apple M1 Pro
• 200 literals, lower-bound prune, median:
  • int32: 3.14 µs -> 0.18 µs
  • string: 3.77 µs -> 0.20 µs
  • decimal: 3.52 µs -> 0.24 µs
• Allocations: int32 192 B / 7 -> 144 B / 6; string 208 B / 8 -> 160 B / 7; decimal 240 B / 10 -> 192 B / 9
• Command: go test ./table -run '^$' -bench '^BenchmarkManifestEvaluatorInPredicate$' -benchmem -benchtime=200ms -count=5

Checks

• go test . ./table ./table/dv ./table/substrait -count=1
• go test -race . ./table -run '^(TestLiteralSetExtrema|TestVisitBoundPredicateRefPassesInExtrema|TestVisitBoundPredicateRefDoesNotAllocate|TestManifestEvaluatorInPredicateExtrema|TestManifestEvaluator|TestManifestEvalVisitorEvalRace)$' -count=1
• go vet ./...

[laskoviymishka]

Nice change — precomputing the IN extrema at bind time and collapsing the manifest check to a single comparison is the right idea, and the benchmark harness makes the intent easy to follow.

I'd hold, though, mostly because the optimization doesn't actually fire where it's supposed to. The literals.Len() > inPredicateLimit guard still runs ahead of the new extrema branch, so any IN set past 200 values short-circuits to rowsMightMatch before we ever look at min/max — which is exactly the large-predicate case the PR is meant to speed up. Moving the guard into the fallback arm fixes it, and a 201-literal test with a disjoint range would keep it fixed.

The other thing I'd want settled here is the boundCompare dispatch. It's a second copy of what getCmpLiteral already does, added only because getCmpLiteral is missing the TimestampNano case — but routing around that leaves VisitEqual/VisitGreater/etc. still panicking on nanosecond-timestamp columns, and removeBoundCheck has the same hole. Adding the case to getCmpLiteral and reusing it finishes the TimestampNano fix and drops ~28 lines.

A few smaller things I'd fix while we're here:

• the extrema comparisons use == 1/== -1, but the Comparator contract is > 0/< 0 — a comparator returning ±2 would silently skip the prune
• min/max named returns shadow the Go builtins and will trip golangci-lint's predeclared
• Negate() copies extrema onto the NOT IN result, where they're never read
• a doc note that VisitInWithExtrema only fires on the borrowed path and only manifestEvalVisitor implements it today

Once the ordering and the dispatch are sorted, happy to take another pass and approve.

[laskoviymishka]

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.

[laskoviymishka]

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.)

[laskoviymishka]

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?

[laskoviymishka]

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?

[laskoviymishka]

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.

[laskoviymishka]

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.

[laskoviymishka]

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.