transform: make literal constraint detection index-directed by frankmcsherry · Pull Request #38515 · MaterializeInc/materialize

frankmcsherry · 2026-08-27T15:16:40Z

LiteralConstraints converts a filter predicate to disjunctive normal form before it consults any index, then reads lookup values off the resulting disjuncts. The expansion is multiplicative in the arity of every disjunction in the predicate, including disjunctions over columns that no index covers. A guard bails out above a predicate size of 1000 nodes, and because it measures the predicate before expanding rather than the expansion it is about to build, it neither bounds the result nor admits cases it could afford. Past the guard the transform silently declines the index.

So this loses the index entirely:

SELECT ... FROM t WHERE shop_id = 'X' AND sku_code IN (<500 values>) AND ...
--                                     index on (shop_id, sku_code)

with a notice recommending an index on (shop_id, has_dual_price_record), which is an artifact of the bail-out rather than advice. Under the guard it works but pays for structure it does not use: an unrelated two-way OR elsewhere in the predicate halves the IN list length the query is allowed to have.

This makes the detection index-directed. For a given list of key expressions, one pass over the predicate yields the values those expressions may take. Predicate structure that says nothing about them costs a single visit and contributes nothing, so the cost no longer depends on how the disjunctions in the rest of the predicate are arranged.

Reading guide

The diff is large but it is mostly deletion, and it splits cleanly in two.

Start with src/transform/src/literal_constraints/key_bounds.rs (new, self-contained). It never touches a MirRelationExpr and never looks at an index. It answers one question about MirScalarExpr: given this predicate, what values can these expressions take?

Read it in this order:

The module doc and the three type definitions. KeyBox is one conjunctive bound with an entry per key field, and KeyBounds::boxes is a disjunction of those. The representation is over key values, not predicate syntax, which is the whole point: a IN (1,2) AND b IN (3,4) is one box denoting four key values where a DNF would write four disjuncts, while (a,b) IN ((1,3),(2,4)) needs two, since one box would admit (1,4).
extract and leaf — the interpreter, five cases.
top vs unit. These differ only in exact, and the difference carries the correctness of constraint removal. top means "I could not read this predicate" and must stay in the filter. unit means "there is nothing here to read" and is the identity of and.
and / disjunction / product / intersect — the combinators. disjunction is deliberately n-ary: folding it pairwise normalizes the accumulator once per argument, which is quadratic in IN-list width.
normalize — merges boxes differing in a single field. This is a precision mechanism, not only a cost one. Without it a 500-element IN list is 500 boxes, blows MAX_BOXES, and gets widened, which loses exact and so loses constraint removal.
The readout methods, which are the only surface the transform uses: bounds_every_field, lookup_values, bounded_fields, is_unsatisfiable, exact.
prune_unsatisfiable at the bottom, which is a separate concern (see below).

Then src/transform/src/literal_constraints.rs. Six functions are deleted outright, all DNF machinery: distribute_and_over_or, unary_and, list_of_predicates_to_and_of_predicates, canonicalize_predicates, remove_impossible_or_args, predicates_size. So are the undo_preparation closure and the orig_mfp clone it compared against. Those existed because the old design rewrote the predicate in order to read it: once the MFP is distorted you owe an undo, the undo is lossy, and you need a heuristic to pick between the two results. Nothing is rewritten now, so all of it goes.

Three functions survive. inline_literal_constraints is untouched and still needed, now run on a clone. detect_literal_constraints keeps its signature and job with a new body. remove_literal_constraints goes from about sixty lines to four: drop each predicate whose own bounds are exact.

The call graph in action is four questions, each mapped to a decision:

literal_constrained_exprs(preds)        -> what does the predicate pin anywhere?
prune_unsatisfiable(preds, constrained) -> (pruned, empty)
detect_literal_constraints(..)          -> per index: KeyBounds::conjunction(preds, index_key)
remove_literal_constraints(mfp, key)    -> per predicate: .exact() -> drop it

Treating "every literal-pinned expression" as a key asks the same question at maximum width, which is why one traversal proves whole-predicate contradictions, drives the pruning, and computes the notice's recommended key.

Soundness

Three properties are structural rather than enforced by hand.

Index-directedness. key_bounds only ever sees the expressions it is handed. There is no path by which it can be charged for a disjunction the index does not cover.

Direction. Every rule only widens the admissible key set, so boxes is always a superset of the truth. Over-approximating is therefore always safe for lookups, because the residual filter still runs and the constant collection has distinct rows so the semi-join cannot duplicate. It also makes is_unsatisfiable a valid emptiness proof: an empty superset means an empty set.

Removal gated on one bit. exact is the only thing licensing a rewrite. Dropping an exact predicate is sound because the lookup values intersect what every predicate implies, including retained ones, so a retained predicate can only narrow the key further, never widen it past what a dropped one allowed.

Behaviors kept explicitly

The normal form provided two things as side effects, and both fired whether or not an index existed, since remove_impossible_or_args alone used to trigger the undo. I checked each against tables with no index at all to isolate them from index selection.

Unaffected, because MirScalarExpr::reduce and undistribute_and_or already do them outside this transform: absorption ((a=1 AND b) OR a=1 to a=1), factoring ((a=1 AND b) OR (a=1 AND c) to a=1 AND (b OR c)), and AND-argument dedup.

Kept explicitly: pruning contradictory disjuncts, which prune_unsatisfiable now does in a bottom-up pass reusing the same bounds. It threads each node's bounds back up so no subtree is analyzed twice, and it takes the predicate list as one implicit conjunction so it also catches contradictions spanning two predicates, such as c IN (1,2) alongside c IN (3,4). It subsumes a separate emptiness pass, so it costs about 5%.

A literal null or false now reads as unsatisfiable. Sound for the AND/OR trees that reach us because max(a,b) = true iff either argument is, and min(a,b) = true iff both are, so neither distinguishes false from null when asking whether a result is true. reduce has already pushed NOT to the leaves, where the analysis treats it as opaque. A literal error is deliberately excluded, since that row errors rather than being filtered.

Effect

For the shape above, on a two-column index: a 500-value lookup with both key constraints out of the filter, where before the index was declined. The cross product of two covered IN lists is unchanged, since those lookup values are inherent to a compound key. Ten unrelated two-way ORs, a DNF factor of 1024, produce the same two lookup values as before and a cleaner residual.

Optimizer time for foo = X AND foo2 IN (N values) plus three unrelated conjuncts, debug build, LiteralConstraints only:

N	before	after
100	2413µs	882µs
300	6182µs	2264µs
1000	9186µs	7951µs
3000	27406µs	25260µs

Linear in N now. At N of 1000 and above the old code bailed on the guard, so its time looks reasonable while the plan is wrong. The larger effect is next door: CanonicalizeMfp drops from 21ms to 0.9ms at N of 1000 and from 93ms to 2.4ms at N of 3000, because it no longer inherits a mangled MFP.

Commits

Split so that functionality and expectation churn stay separable.

transform: make literal constraint detection index-directed — the change, plus new sqllogictest coverage for an IN list on an uncovered column, several such lists at once, a covered list past any workable size guard, and the distinction between an inherent and an incidental cross product.
transform: update expectations for index-directed literal constraints — two golden rewrites, both improvements. WHERE a = NULL OR a = 2 now serves the equality from a lookup instead of a full scan, and its paired data query is unchanged so the null row is still not returned. The join equivalence {#0,#1,#2} over a single input lowers to null-safe pairwise equalities with one mapped expression; there is no index on that source at all, so the old expectation recorded only the distortion.

Open questions

MAX_LOOKUP_VALUES is a cap that does not exist today, and nothing in the suite reaches it. It is a guess at where a full scan beats a constant collection and probably wants to be a system variable.
Contradiction pruning is predicate simplification living in an index-selection transform. That is where it lives today too, but it is worth deciding deliberately rather than by inheritance.
The .slt expectations were generated against an older main and the branch was rebased onto current main afterward, so unrelated plan-rendering drift would show up as sqllogictest diffs. Leaving that to CI, with corrections as follow-up commits.

`LiteralConstraints` converts a filter predicate to disjunctive normal form before consulting any index, then reads lookup values off the disjuncts. The expansion is multiplicative in the arity of every disjunction in the predicate, including disjunctions over columns that no index covers, so a query pairing an `IN` list with any other `OR` can expand far past what its answer requires. A guard bails out above a predicate size of 1000 nodes, and because it measures the predicate before expanding rather than the expansion it is about to build, it neither bounds the result nor admits cases it could afford. Past the guard the transform silently declines the index, so `shop_id = X AND sku_code IN (<500 values>)` on an index over `(shop_id, sku_code)` falls back to a full scan. Ask the question per candidate index instead. For a given list of key expressions, one pass over the predicate yields the values those expressions may take, and predicate structure that says nothing about them costs a single visit and contributes nothing. The answer is a disjunction of conjunctive boxes, where a box bounds each key field independently, so `a IN (..) AND b IN (..)` is one box denoting the cross product rather than a disjunct per pair, and the box count is bounded by the number of distinct key tuples the predicate admits rather than by how its disjunctions are arranged. Nothing is rewritten in order to be read, which removes the preparation and its lossy undo along with the heuristic that chose between them. Constraint removal becomes a per-predicate test: a predicate that is exactly a bound on the key can go, which is sound because the lookup values intersect what every predicate implies, so a retained predicate can only narrow the key further. Two behaviors that the normal form used to provide as side effects are kept explicitly. Contradictory disjuncts are pruned by a bottom-up pass that reuses the same bounds, covering contradictions that span two predicates. A literal `null` or `false` now reads as unsatisfiable, which is sound for the `AND`/`OR` trees that reach us because neither `min` nor `max` distinguishes `false` from `null` when asking whether a result is `true`, and a filter drops a `null` row just as it drops a `false` one. Adds sqllogictest coverage for an `IN` list on a column the index does not cover, for several such lists at once, for a covered list long enough to exceed any workable size guard, and for the distinction between a cross product that is inherent to a compound key and one that is incidental.

Two plans improve. `WHERE a = NULL OR a = 2` reduces to a `null` literal disjoined with a literal equality. A filter drops a `null` row just as it drops a `false` one, so the predicate is equivalent to `a = 2` and the equality can be served by a lookup rather than a full scan with a residual filter. The paired data query is unchanged, so the `null` row in the table is still not returned, which is what the case was written to check. The join equivalence `{#0, #1, #2}` over a single input lowers to the null-safe pairwise equalities with one mapped expression, rather than the shape the old normal-form round trip left behind. There is no index on that source at all, so the previous expectation recorded only the distortion: the expansion ran, found nothing to use, and the node-count heuristic then preferred the tangled result over the predicate it started from.

antiguru

Skimmed it, seems fine!

antiguru

The design is right and the deletion is earned. KeyBounds answers the question the transform actually has, the DNF machinery genuinely was answering a different one, and I checked both golden changes by hand: the join-equivalence rewrite is the null-safe pairwise form and agrees with the old one on every null combination, and a = NULL OR a = 2 really is equivalent to a = 2 as a filter, so serving it from a lookup is correct.

One blocking correctness bug, then smaller things.

Blocking: wrong results from `and()` widening

remove_literal_constraints drops each predicate whose own bounds are exact, on the precondition stated in its NOTE: the lookup values intersect what every predicate implies. KeyBounds::and breaks that when the product of two box lists exceeds MAX_BOXES. It widens one operand to its bounding box and clears exact on the result, but the operands keep their own exact, and removal never consults the conjunction's flag.

With an index on (a, b):

WHERE (a,b) IN ((0,0),(1,1),...,(39,39))
  AND (a,b) IN ((0,1),(1,2),...,(39,40))

40x40 = 1600 > MAX_BOXES, so the first list collapses to {a: 0..39} x {b: 0..39}, the second survives intact, both predicates report exact and are removed, and the plan looks up 39 key values for a predicate whose true answer is zero rows.

Confirmed against this branch with a unit test on KeyBounds alone, asserting the property removal depends on (the lookup values of a conjunction fall inside the bounds of every exact conjunct):

test key_bounds::tests::exact_conjunct_contains_lookup_values ... FAILED
lookup values not implied by the predicate:
  {[0,1], [1,2], [2,3], ... [38,39]}    (39 values)

Both assert!(exact) lines preceding it pass. The test is on claude/pr-38515-review-iv2qom (commit 2f38d01) if it is useful.

About 33 pairs per list is enough to trip it, and (a,b) IN (...) AND (b,c) IN (...) on a three-column index is the same shape and more likely to show up in generated SQL. Note the other two consumers are fine: is_unsatisfiable and the lookup values only ever over-approximate, and widening preserves that. It is specifically the removal step. Smallest fix is a widened bit out of KeyBounds::conjunction, gating remove_literal_constraints.

Behavior worth deciding rather than inheriting

Error suppression widens. prune_inner rewrites any unsatisfiable subtree to false, and empty replaces the whole relation, so WHERE a IN (1,2) AND a IN (3,4) AND (1/x > 0) returns empty where the division would have errored. remove_impossible_or_args did the same for a whole OR arg, so it is not new in kind, but it now also reaches contradictions spanning two separate predicates, which the old pass only caught after DNF. Worth a line in the "Behaviors kept explicitly" section.

Literal identity is Row byte identity. RowRef's Ord carries "Warning: These order by the u8 array representation, and NOT by Datum::cmp". munge_numeric normalizes -0 but not scale, so n = 1.0 AND n = 1.00 intersects to the empty set and the relation is declared empty, though 1.0 = 1.00 is true. Pre-existing (the old all_unique() check reached the same verdict) and not this PR's to fix, but the new representation leans on set intersection much harder, so the assumption deserves a line in the module doc.

Cost

The table is single-column IN lists, which is the case the new design is fastest on. Two shapes it does not cover:

recommended_key is now computed on every result.is_none(), including when the Get has no indexes at all. That is a full bounds pass over constrained, the widest key in play, on the common path. The old code computed it inside the UnusableTooWide arm; gating on index_matches.iter().any(...) would restore that.
normalize runs on every product, and its field loop is 0..arity with KeyBox comparisons that are O(arity). For (a,b) IN (1000 pairs) AND c1 = 1 AND ... AND c48 = 48, each of the 48 folds re-sorts and re-groups 1000 boxes across 50 fields. I did not measure this, so treat it as a shape to benchmark rather than a finding, but the old 1000-node guard is gone and nothing replaced it, so it is worth knowing where the new ceiling sits.

Smaller

lookup_values opens with assert!(self.bounds_every_field()), and the notice path reaches it via bounded_fields computed against a different key list. I convinced myself the invariant holds (projection, and both widenings, preserve Some-ness), but an optimizer panic is a rough failure mode for a cross-call invariant. Returning None would be safer.
literal_constrained_exprs's doc says it is used for "cheaply rejecting an index whose key mentions an expression the predicate never pins, and recommending a key". match_index never consults constrained; only the recommendation exists.
exact means "equivalent as a filter", which leaf's NOTE says but the field doc on exact does not. Worth stating where the definition lives, since it is what licenses treating a literal null as unsatisfiable.
MAX_LOOKUP_VALUES is far more permissive than the old size guard (which bailed around 140 pairs), so I could not find a regression window from adding it.

CI

buildkite/test/doctests is red on 729d3d0. That step is ci/test/lint-doc.sh: bin/doc --document-private-items plus ci-closed-issues-detect, cargo about, helm unittest, bin/pydoc. I checked the closed-issue detector against the new database-issues/issues/1924 reference, and it only fires on comment blocks containing TODO/reenable/in the future, none of which that block has, so rustdoc looks more likely. I did not reproduce it.

I also could not enumerate all 81 statuses through the API, so I do not know whether the sqllogictest steps that own literal_constraints.slt are green. Given the note about the expectations predating the rebase, that is the first thing I would check.

Generated by Claude Code

antiguru · 2026-08-27T19:07:25Z

+        // Widen before multiplying, so the product stays inside the budget. Only the wider
+        // operand is widened, because widening both would discard structure that
+        // `MAX_BOXES` can still afford to keep.
+        let (left, right, exact) = if self.boxes.len() * other.boxes.len() > MAX_BOXES {
+            if self.boxes.len() >= other.boxes.len() {
+                (Self::widen(&self.boxes, arity), other.boxes, false)
+            } else {
+                (self.boxes, Self::widen(&other.boxes, arity), false)
+            }
+        } else {
+            (self.boxes, other.boxes, self.exact && other.exact)
+        };


This widening is where the removal precondition breaks (see the review body for the failing case).

Widening clears exact on the result, but the operands keep theirs, and remove_literal_constraints only ever looks at a predicate's own exact. So the accumulator gets replaced by its bounding box and the predicate that produced it is dropped from the filter without the lookup enforcing it.

Suggest carrying a widened bit out of KeyBounds::conjunction and gating remove_literal_constraints on it. is_unsatisfiable and the lookup values themselves stay sound under widening, so nothing else needs the bit.

Separately: this widens the wider operand. Widening the narrower one is equally within budget (the product is then max(len) <= MAX_BOXES, by the invariant both branches already maintain) and keeps strictly more structure. With 1000 boxes against 2, the current choice discards the 1000-way partition to keep a 2-way one. The comment defends the choice against widening both, which isn't the alternative in question.

Generated by Claude Code

antiguru · 2026-08-27T19:07:35Z

+        let (map, predicates, project) = mfp.as_map_filter_project();
+        let kept = predicates
+            .into_iter()
+            .filter(|p| !KeyBounds::extract(p, key).exact())


The NOTE above this function states the precondition exactly right:

the lookup values are the intersection of what every predicate implies

KeyBounds::and violates it when it widens (see the comment on key_bounds.rs). A per-predicate exact is not enough on its own; it needs pairing with "and no widening happened while folding the conjunction".

Also on the doc: (f1 = 3 AND f2 = 5) OR (f1 = 7 AND f2 = 5) with key f1 is described as not removable "because the residual f2 = 5 is entangled with the f1 constraint". It isn't entangled, the expression is f1 IN (3,7) AND f2 = 5, and the code being deleted here did remove it. In practice undistribute_and_or factors this shape upstream before the transform sees it, so I don't think anything regresses, but the doc explains a capability loss as a necessary limitation.

Generated by Claude Code

`bin/doc` runs rustdoc with `-D warnings`, which rejects an intra-doc link from a public module to a private one even under `--document-private-items`, since the link would break without that flag. Refer to the `key_bounds` submodule in prose instead. Also drop a reference to a closed issue from a comment on a new test. `bin/ci-closed-issues-detect --changed-lines-only` flags those, and the case the reference belonged to already cites it a few lines up.

Removing a predicate that exactly bounds the index key is sound only because the lookup values are the *intersection* of what every predicate implies, so they fall inside the removed predicate's own bounds. `KeyBounds::and` breaks that when a box product exceeds its budget: it widens one operand to the box containing it, which yields a superset of the intersection. The operands keep their own `exact` flags, and removal consulted only those, so both predicates of a two-list conjunction could be dropped against lookup values that neither of them admits. With an index on `(a, b)` and two disjoint 40-element pair lists, the plan looked up 39 keys with no residual filter and returned rows for a predicate whose answer is empty. Track widening as its own bit rather than folding it into `exact`. `exact` has to stay per-predicate for removal to work at all, since a conjunction containing one opaque predicate is not exact yet its other conjuncts are still removable. Widening is a property of the conjunction, so removal now refuses outright when the bound it is working against widened anywhere. Keeping the filter is always correct, as the lookup then merely over-approximates. The other two consumers were never affected: lookup values and `is_unsatisfiable` only ever over-approximate, which widening preserves. Adds a sqllogictest asserting rows rather than a plan, since over-approximating the lookup is allowed and returning a row is not, and unit tests on `KeyBounds` for the widened and non-widened conjunctions. Also skips the no-op passes in `normalize`. Merging on a field that every box agrees on cannot combine anything, because two boxes sharing a group would then agree everywhere and have been deduplicated already. Wide keys are mostly pinned to single values, so this is nearly all of them: for a 1000-value tuple list conjoined with 48 equalities, `LiteralConstraints` drops from 245ms to 49ms, against 23ms before this branch, where no index was selected at all. Gates the "index too wide" recommendation on some index actually being too wide. It runs a bounds pass over the widest key in play, and a Get with no indexes should not pay for it. Replaces an assertion in `lookup_values` with a `None`, since a panic is a poor failure mode for an invariant that spans calls.

frankmcsherry · 2026-08-28T19:34:48Z

Confirmed the blocking bug, fixed it, and worked through the rest. Thank you — this was a real wrong-results bug, not just a bad plan.

The bug

Reproduced as a data query before touching anything. With an index on (a, b) and your two disjoint 40-element lists, against rows (5,5), (5,6), (7,8):

expected: []
actually: [(5,6), (7,8)]

The plan had no residual filter at all, so 39 widened lookup values went straight to the output. Your diagnosis is exactly right: and clears exact on the result while the operands keep theirs, and remove_literal_constraints only ever consulted the per-predicate flag.

Fixed with the widened bit you suggested, propagated through and and disjunction and gated in remove_literal_constraints. Two notes on the shape of the fix:

exact could not simply absorb it. exact has to stay per-predicate or removal stops working in the common case, since a conjunction containing one opaque predicate is not exact while its other conjuncts are still removable. Widening is a property of the conjunction, so it needs its own bit consulted at a different level.

The gate is deliberately conservative: any widening anywhere refuses all removal, even though removal of the un-widened operand stays sound (the product is still contained in it). Buying that back means tracking containment per predicate through the fold, which did not seem worth the subtlety for a case that is already degrading gracefully.

Regression tests at both levels. The sqllogictest asserts rows rather than a plan, since over-approximating the lookup is allowed and returning a row is not. The unit tests assert widened() on the 40x40 conjunction while both operands report exact(); I mutation-checked them by flipping the true back to false in and, and widening_in_a_conjunction_is_reported fails as it should.

normalize cost

You were right to flag it, and it was worse than you guessed. Your exact shape does not plan — a 1000-element tuple list hits a recursion limit in the SQL desugarer, before the optimizer — so I measured what does, (a,b) IN (n pairs) AND c1 = 1 AND ... AND c48 = 48, LiteralConstraints only, debug build:

pairs	main	this branch, before	this branch, now
50	16.3ms	46.0ms	14.1ms
100	24.1ms	108.0ms	25.7ms
200	23.2ms	245.3ms	49.1ms

A 10x regression at 200 pairs, now gone. The fix is that merging on a field every box agrees on cannot combine anything: two boxes sharing a group would then agree on every field and have been deduplicated already. A wide key is mostly pinned to single values, so that is nearly every field — 48 of 50 here. End-to-end the shape is now 404ms against main's 425ms, while producing an index plan where main selects none. Single-column lists got slightly faster too.

I have not found the ceiling, only moved it. The remaining cost is intersect cloning a BTreeSet per field per box pair, which the owned representation makes unavoidable without more work.

Also fixed

recommended_key on the common path. Gated on some index actually reporting UnusableTooWide, so a Get with no indexes no longer pays for a bounds pass over the widest key in play.
lookup_values assertion. Now returns None. Agreed that an optimizer panic is a rough failure mode for a cross-call invariant, even a holding one.
Error suppression. Documented on prune_unsatisfiable, including that the reach is wider than remove_impossible_or_args because contradictions spanning two predicates are now visible. I will add the line to the PR body too.
Row byte identity. Documented in the module doc, with the 1.0 / 1.00 case named. Agreed it is pre-existing and not this PR's to fix, but you are right that the new representation leans on it much harder.
literal_constrained_exprs doc no longer claims a use that does not exist, and exact now says "equivalent as a filter" where the field is defined, not only in leaf's note.

CI

doctests was rustdoc, as you suspected: a public module's doc linking to the now-private key_bounds, which -D warnings rejects even under --document-private-items because the link would break without it. Fixed in 1a83451.

On the closed-issue detector you were right and I was wrong. I had removed the issues/1924 reference expecting it to fire; it only triggers on comment blocks containing TODO / reenable / in the future and similar, none of which that block has. Verified by restoring the reference and running the detector, which passes. It is back.

All five slt-* shards were green on 729d3d0, so the expectations do hold across the rebase.

ggevay · 2026-09-01T13:51:39Z

(Triggered a Nightly: https://buildkite.com/materialize/nightly/builds/18196)

Edit: Ehh, tons of Nightly failures, but so far all look unrelated at first glance. I'm triaging them now.

Edit 2: All Nightly failures are unrelated. I've opened various issues, and extended some ci-regexp.

ggevay

Thank you! LGTM, just minor comments.

ggevay · 2026-09-01T12:26:48Z

+//! NOTE: Literal values are compared as `Row`s, and `RowRef`'s `Ord` orders by the packed
+//! byte representation rather than by `Datum::cmp`. Two literals that compare equal in SQL
+//! can therefore land in different set elements: `munge_numeric` normalizes `-0` but not
+//! scale, so `n = 1.0 AND n = 1.00` intersects to the empty set even though `1.0 = 1.00`.
+//! The verdict is the same one the surrounding transform has always reached, but the set
+//! arithmetic here leans on it much harder, so treat byte identity as the definition of
+//! literal equality for these purposes.


This example is wrong: push_datum reduces numerics before writing them (row.rs:2078-2083), so 1.0 and 1.00 are byte-identical, and on v26.39 n = 1.0 AND n = 1.00 plans to lookup value=(1) and returns both rows. Floats are the live case: Float64 packs raw bits (row.rs:1986-1989), so f = 0.0 AND f = '-0'::float8 intersects to empty and returns 0 rows where 2 are correct (SQL-452, fix in #37482).

Suggested change

//! NOTE: Literal values are compared as `Row`s, and `RowRef`'s `Ord` orders by the packed

//! byte representation rather than by `Datum::cmp`. Two literals that compare equal in SQL

//! can therefore land in different set elements: `munge_numeric` normalizes `-0` but not

//! scale, so `n = 1.0 AND n = 1.00` intersects to the empty set even though `1.0 = 1.00`.

//! The verdict is the same one the surrounding transform has always reached, but the set

//! arithmetic here leans on it much harder, so treat byte identity as the definition of

//! literal equality for these purposes.

//! NOTE: Literal values are compared as `Row`s, and `RowRef`'s `Ord` orders by the packed

//! byte representation rather than by `Datum::cmp`. Packing canonicalizes numerics, but writes

//! a float's raw bits, so `f = 0.0 AND f = '-0'::float8` intersects to the empty set even

//! though the two literals are equal in SQL (https://linear.app/materializeinc/issue/SQL-452).

//! Byte identity is therefore the definition of literal equality for everything in this module.

ggevay · 2026-09-01T12:26:48Z

+
+/// Largest number of key values we will ask an index to look up. Above this a full scan
+/// plus filter is the better plan, and the constant collection would itself be a burden.
+const MAX_LOOKUP_VALUES: usize = 100_000;


How was the 100_000 determined? I guess the "Above this a full scan plus filter is the better plan" is not universally true, right? Also, could you make it a system var, if it's not too hard?

(The old code could do ~330.)

ggevay · 2026-09-01T12:26:49Z

+            // Disjuncts that contradict themselves are dead weight in the filter, and
+            // pruning them needs no index. Done before detection so that detection sees the
+            // simplified predicate.
+            let (pruned, empty) = Self::prune_unsatisfiable(&mut probe_mfp, &constrained);


This pass keys on every literal-pinned expression, so its arity is the number of distinct equalities in the predicate (50 in the (a,b) IN (...) AND c1 = 1 ... AND c48 = 48 shape from your table), and the work per predicate fold scales with that arity: leaf tries every key field with a clone on each miss (scalar.rs:384), and intersect clones every bounded field's set for every box (key_bounds.rs:332).

Only an expression pinned to two or more distinct literals can ever empty a field (a field with one value intersects to itself, and the false/null/impossible leaves are bottom regardless of key), so the pruning key can be restricted to those, which makes the pass scale with the number of contradiction candidates instead. constrained stays as is for the recommendation.

The comment at 249-251 ("a Get with no indexes at all must not pay for it") is contradicted by this line, since it is the same-arity pass.

ggevay · 2026-09-01T12:26:49Z

+                            let bounds = LiteralConstraints::key_bounds(mfp, &usable_subset);
+                            let Some(literal_values) = bounds.lookup_values() else {
+                                return;
                            };


Cap what goes into the notice independently of MAX_LOOKUP_VALUES: fmt_message renders every value and the string is persisted into mz_optimizer_notices for index and MV dataflows. A dozen values plus a count is enough for a hint.

ggevay · 2026-09-01T12:26:49Z

+///
+/// `predicates` is an implicit conjunction, as an MFP's predicate list is.
+///
+/// This is what turns `a IN (1, 2) AND a IN (2, 3, 4)` into `a = 2`, and it applies whether


prune_unsatisfiable does not turn a IN (1, 2) AND a IN (2, 3, 4) into a = 2: neither list nor the conjunction is unsatisfiable, so changed stays false, and the single lookup value comes from detect + remove. An example this function rewrites is (a = 1 AND a = 2) OR b = 3 -> false OR b = 3.

FYI, on a table with no index main simplifies the IN-list pair to Filter (a = 2) (via the old DNF) and this branch leaves both lists; fine, but the doc should not claim otherwise.

ggevay · 2026-09-01T12:26:49Z

+        for b in &self.boxes {
+            let sets = b.iter().map(|f| f.as_ref()).collect::<Option<Vec<_>>>()?;
+            for combination in sets.into_iter().multi_cartesian_product() {
+                values.insert(Row::pack(combination.iter().map(|r| r.unpack_first())));
+                if values.len() > MAX_LOOKUP_VALUES {
+                    return None;
+                }
+            }


nit: the cap tests the deduped set, not the iterations; overlapping boxes can enumerate far more than MAX_LOOKUP_VALUES combinations before returning. Counting iterations is the same line.

ggevay · 2026-09-01T12:26:49Z

+    /// dropping it loses nothing. `(f1 = 3 AND f2 = 5) OR (f1 = 7 AND f2 = 5)` with a key
+    /// of just `f1` is not removable, because the residual `f2 = 5` is entangled with the
+    /// `f1` constraint. `f1 IN (3, 7) AND f2 = 5` is: the first predicate goes, the second
+    /// stays.


nit: the example is not entangled (f2 = 5 is the same in both disjuncts; the old code removed it) and the mechanism is different: and clears exact on any non-key conjunct (key_bounds.rs:221), so the genuinely entangled (f1 = 3 AND f2 = 5) OR (f1 = 7 AND f2 = 555) is declined for the same reason. Upstream undistribute_and_or usually factors the first shape, which is why no golden moved; worth saying rather than explaining a capability gap as a necessity.

ggevay · 2026-09-01T12:26:49Z

+    /// Whether the derivation widened anywhere, which makes the bound a strict
+    /// over-approximation of what the predicate implies.
+    ///
+    /// Removal reasons about *containment*, not just exactness: dropping a predicate that is
+    /// exactly a bound on the key is sound only because the lookup values are the
+    /// intersection of what every predicate implies, so they fall inside the dropped
+    /// predicate's own bounds. Widening produces a superset of that intersection instead, so
+    /// a lookup value need no longer satisfy a dropped predicate, and the rows it finds would
+    /// go unfiltered. Callers that remove must refuse when this is set.


nit: the containment argument is stated here, in the field doc (88-95), the module NOTE (29-33) and the remove_literal_constraints NOTE (337-343). Let this one own it and point the others here. Same for the "structure the index does not cover costs one visit" fact (kb 14-15, 26-27, lc 15-17, 185-188) and the "over-approximation is safe" fact (kb 29-33, 421-422, lc 342-343).

And the boxes.len() <= MAX_BOXES invariant that keeps product bounded is load-bearing but unstated; one line on the field.

ggevay · 2026-09-01T12:26:49Z

+        if boxes.is_empty() {
+            // "Never satisfied" needs no widening, and widening it would be wrong.
+            return Vec::new();
+        }


nit: unreachable. and widens only when the product exceeds MAX_BOXES (impossible with a zero factor) and disjunction only when the count does. Either drop the branch or say it is defensive.

ggevay · 2026-09-01T12:26:49Z

+# The same shape under the budget, where the intersection is exact and stays empty.
+
+query II rowsort
+SELECT * FROM pairs WHERE (a,b) IN ((0,0),(5,5)) AND (a,b) IN ((0,1),(5,6))


Tests worth adding: the MAX_LOOKUP_VALUES cap (unit), disjunction-level widening (unit, pair_list(2000)), (a = NULL AND b = 'l1') OR a = 2 (main full-scans with null AND left in the filter, this branch should look up (2)), and a pruned-only case on a table with no index.

frankmcsherry added 2 commits August 27, 2026 11:14

frankmcsherry requested a review from ggevay August 27, 2026 15:31

antiguru approved these changes Aug 27, 2026

View reviewed changes

antiguru reviewed Aug 27, 2026

View reviewed changes

frankmcsherry added 2 commits August 28, 2026 15:05

ggevay approved these changes Sep 1, 2026

View reviewed changes

frankmcsherry mentioned this pull request Sep 8, 2026

transform: detect index lookups from the filter as written #38717

Draft

ggevay mentioned this pull request Sep 10, 2026

repr: canonicalize float encodings in row packing #37482

Draft

Uh oh!

Conversation

frankmcsherry commented Aug 27, 2026

Reading guide

Soundness

Behaviors kept explicitly

Effect

Commits

Open questions

Uh oh!

antiguru left a comment

Choose a reason for hiding this comment

Uh oh!

antiguru left a comment

Choose a reason for hiding this comment

Blocking: wrong results from and() widening

Behavior worth deciding rather than inheriting

Cost

Smaller

CI

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frankmcsherry commented Aug 28, 2026

The bug

normalize cost

Also fixed

CI

Uh oh!

ggevay commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggevay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Blocking: wrong results from `and()` widening

ggevay commented Sep 1, 2026 •

edited

Loading