fix(query): range-filter pushdown keeps every conjunct; push xsd:decimal bounds - #1778
Conversation
`extract_range_constraints` skipped any AND conjunct that yielded no constraint and still returned the ones it could represent. The pushdown consumer then dropped the whole FILTER as "fully pushed" because every variable in it had a bound, so the skipped conjunct was never evaluated. The common trigger is a bare SPARQL literal like `0.01`, which is xsd:decimal and has no `RangeValue`: `FILTER(?v >= 0 && ?v < 0.01)` returned every row with `?v >= 0`. Booleans or any other constant that `extract_const` rejects behave the same way. Now a conjunct that fails to extract fails the whole expression, so the filter is kept and evaluated in full. Queries whose conjuncts all extract plan exactly as before. Adds planner unit tests for the mixed-conjunct, reversed, nested and boolean shapes, plus an end-to-end SPARQL test over the six-row repro from the report. All fail without the fix.
Add `RangeValue::Decimal` so a bare SPARQL literal like `0.01` pushes into the scan-side object bounds instead of forcing the whole FILTER back onto the generic lane. The scan already compares bounds with the class-aware `ObjectBounds::matches`, so no encoding work is needed. `compare_range_values` now orders values within their class and returns `None` for pairs that have no order, using the same `numeric_cmp` and `temporal_cmp` the scan uses. It previously fell back to `Equal` for every pair it did not list, which included all temporal pairs and any cross-class pair. With `Equal`, `merge` kept whichever bound it saw first, so `?d >= 2020-01-01 && ?d >= 2021-06-01` pushed the 2020 bound and consumed the filter, returning rows the 2021 bound excluded. `RangeConstraint::merge` now reports failure when two same-side bounds cannot be ordered, and both extraction paths bail to "no pushdown" in that case. `is_unsatisfiable` treats an unordered pair as satisfiable; the scan's class check rejects every row for it anyway. Tests: planner units for the mixed decimal shape, cross-type tighter merges, cross-type contradiction, the temporal same-side merge, and unordered same-side bounds. The end-to-end SPARQL table gains same-side, contradictory and date cases and now runs over both a novelty-only ledger and an indexed one, asserting the index landed. The temporal cases fail in both lanes without the comparator change.
bplatz
added
P1
aaj3f
left a comment
aaj3f
left a comment
There was a problem hiding this comment.
@bplatz good catch & good fix. The kind where the shape (?v >= 0 && ?v < 0.01) is so ordinary that silently returning every ?v >= 0 row is the worst possible failure, and the diagnosis is exactly right on both counts: the And arm dropped what it couldn't represent while the consumer's per-variable check couldn't see a missing conjunct, and the comparator's Equal fallback let merge keep whichever temporal bound came first. I verified both rather than reading them: restoring the conjunct skip reddens the two planner unit tests, and restoring the Equal fallback for temporal pairs reddens both e2e tests on precisely the >= 2020 && >= 2021 case in the novelty and indexed lanes. Making compare_range_values route through the same numeric_cmp/temporal_cmp the scan's ObjectBounds::matches (and the generic comparator) already use is the part I like most — it closes the class of planner-vs-scan disagreement rather than this instance, and RangeValue::Decimal just falls out of it.
One small thing I'd fold in: the SPARQL case table no longer pins the total-or-nothing fix on its own — with commit 1 reverted both e2e tests still pass, because commit 2 made every literal in the table representable. A single case with a typed oversized-integer conjunct (?v >= 0 && ?v > "100000000000000000000"^^xsd:integer → []) restores independent teeth; I confirmed it goes red under the reverted arm and green at HEAD. Also worth knowing for the "not in scope" note: the bare 100000000000000000000 form is a parse error today, so that item is only reachable via the typed literal (pre-existing).
Adherence to repo commitments:
- Patterns/abstractions: ✔ fix at the extractor, not a second conjunct walk in the consumer; comparator reuses
numeric_cmp/temporal_cmpshared with the scan and the generic lane; SPARQL/JSON-LD parity preserved by construction (shared IR, IR-level unit tests). - Performance (speed first, memory second): ✔ planner-only, O(conjuncts) per query; scan untouched; mixed decimal filters now take the in-scan bounds lane instead of the generic filter operator. No performance-degradation risk.
- Testing: ✔ seven planner unit tests plus a two-lane SPARQL case table in the declared
grp_query_sparqltarget, mutation-verified here;⚠️ the e2e table needs one unrepresentable-conjunct case to pin defect 1 independently. - Conventions: ✔ two thorough multi-line commits,
P1/area:querylabelled, self-describing title; fmt/clippy green in CI.
Verified locally at branch HEAD 29c99cbf2: cargo test -p fluree-db-query -- planner::tests::… → 33 passed; cargo test -p fluree-db-api --test grp_query_sparql sparql_range_filter → 2 passed; mutation (restore conjunct skip) → 2 unit tests FAILED, e2e still green → teeth note; mutation + typed-BigInt case → e2e FAILED as expected, restored clean; mutation (temporal → Equal) → both e2e FAILED as expected, restored clean; cargo fmt --check clean.
Approving so you can merge when ready, but maybe worth adding that one case first.
| ("ex:price", "?v < 5 && ?v < 0.01", &["b"]), | ||
| ("ex:price", "?v < 0.01 && ?v < 5", &["b"]), | ||
| ("ex:price", "?v > 0.5 && ?v > 0e0", &["a", "c", "e"]), | ||
| // Contradictory across types: empty, not "everything". |
There was a problem hiding this comment.
Should-fix (test teeth, fold in now). The SPARQL table doesn't pin the total-or-nothing fix on its own any more.
With the And arm's conjunct skip put back (planner.rs:631), the two planner unit tests go red but both sparql_range_filter_keeps_every_conjunct_* tests stay green — because after commit 2 every literal in the table is representable (?v < 0.01 now extracts as a decimal bound), so there's no conjunct left to drop.
A case with a parseable-but-unrepresentable conjunct gives it independent teeth. This one passes at HEAD and fails under the reverted arm (returns all six rows):
("ex:price", "?v >= 0 && ?v > \"100000000000000000000\"^^xsd:integer", &[]),One line, and it keeps the e2e honest against a future refactor of the extractor.
| /// Returns `None` for pairs that have no order (numeric vs string, date vs | ||
| /// time, ...) — never a fabricated `Equal`, which would let `merge` keep an | ||
| /// arbitrary bound and silently drop the other. | ||
| fn compare_range_values(a: &RangeValue, b: &RangeValue) -> Option<std::cmp::Ordering> { |
There was a problem hiding this comment.
Praise. Routing numeric pairs through FlakeValue::numeric_cmp and temporal pairs through temporal_cmp — the same functions ObjectBounds::matches and the generic comparator use — is the fix that closes the class rather than the instance: the planner can no longer keep a bound the scan would order differently. Returning None instead of a fabricated Equal is the whole bug.
| /// lower bound). The tighter bound is then unknowable, so the caller must | ||
| /// not push either one down; `self` is left unchanged in that case. | ||
| #[must_use] | ||
| pub fn merge(&mut self, other: &RangeConstraint) -> bool { |
There was a problem hiding this comment.
Praise. Computing both sides into locals and committing only when both merges succeed means a failed merge leaves self untouched, and #[must_use] makes the two callers check it. Worth keeping that shape if the function grows.
| // Bounds of different classes (numeric vs string, ...) are | ||
| // not provably contradictory here; the scan's class-aware | ||
| // `ObjectBounds::matches` rejects every row anyway. | ||
| None => false, |
There was a problem hiding this comment.
Note (no change needed). I traced whether None => false here can let an unordered pair through: it can only be an opposite-side cross-class pair (same-side ones already failed merge), and ObjectBounds::matches rejects every row for it via class_cmp — which is also what SPARQL says for a number-vs-string comparison (type error → excluded). Consistent; the comment already says so.
The SPARQL case table lost its teeth for the first fix once RangeValue gained Decimal: every literal in the table became representable, so restoring the dropped-conjunct skip left both e2e tests green. Only the planner units still caught it. An xsd:integer bound past i64 has no RangeValue at all, so a conjunct carrying one exercises the partial-extraction path the decimal cases no longer reach. Both directions are covered so a lane that returns nothing cannot pass the empty case by accident. Verified non-vacuous: with the conjunct skip restored, both lanes fail on `?v >= 0 && ?v > "1e20"^^xsd:integer`, returning all six rows instead of none.
xsd:integer is unbounded, and the lexer has emitted a distinct `BigInteger` token past i64 for a while. Term position consumes it, both signed and unsigned, and lowering promotes it to `FlakeValue::BigInt`. Expression position was the one hole: `try_parse_literal` matched Integer, Decimal, Double, String and Boolean but had no `BigInteger` arm, so `FILTER(?v > 100000000000000000000)` failed with "Expected expression" while the identical bound written as a typed literal parsed fine. Adds the missing `consume_big_integer` helper alongside its Integer and Decimal twins and the arm that uses it. The sign needs no special handling here: unlike term position, expressions parse `-` as the unary operator. This only closes the parse hole. Bounds past i64 still have no `RangeValue`, so they do not push down; the filter stays and is evaluated generically, which is the correct-and-slower path already described in #1335.
Accepting BigInteger in expression position made lowering paths reachable that a bare oversized literal could not previously reach. All five LiteralValue match sites already had a BigInteger arm, but that was worth exercising rather than reading: BIND, arithmetic, unary negation, ORDER BY and HAVING each route through a different one.
|
Thanks @aaj3f — both parts of that were right, and I verified rather than taking them on faith. The teeth gap is real. Restoring the conjunct skip and re-running the e2e pair: both still green, exactly as you said. The bare-form aside was also correct, though for a narrower reason than "parse error" suggests — and chasing it turned up something worth fixing: The lexer has emitted a distinct Fixed in Since widening the parser makes previously-unreachable lowering paths reachable, I checked all five I ran the workspace-excluded Also folded in while here: Body updated throughout, including the Tests section, which had been claiming all new tests failed against the pre-fix code — the exact claim that had stopped holding. Re-review is welcome given the parser commit landed after your approval, though @bplatz was happy to fold it in rather than split it out. |
2 participants
Summary
A
FILTERthat conjoins two bounds on one variable silently dropped any bound the planner could not represent as aRangeValue, most commonly anxsd:decimalliteral such as0.01.FILTER(?v >= 0 && ?v < 0.01)returned every row with?v >= 0. A SHACL rule built on this shape flagged 193,337 of 195,338 receipt lines instead of 492.Two defects, fixed in two commits:
Partial pushdown replaced the whole filter. The
Andbranch ofextract_range_constraintsskipped conjuncts that yielded no constraint and still returned the rest. The pushdown consumer then marked the filter consumed because every variable had a bound, without checking that every conjunct contributed. Now a conjunct that fails to extract fails the whole expression, so the filter is kept and evaluated in full. Queries whose conjuncts all extract plan exactly as before.Bounds of unlisted types compared as
Equal.compare_range_valuesfell back toEqualfor any pair it did not list, which included every temporal pair.mergethen kept whichever same-side bound it saw first:?d >= 2020-01-01 && ?d >= 2021-06-01pushed the 2020 bound and consumed the filter. The comparator now orders within a class via the samenumeric_cmp/temporal_cmpthe scan uses and returnsNonefor unordered pairs;mergereports failure on an unordered same-side pair and both extraction paths bail to "no pushdown".A fourth commit closes an adjacent parser hole found while testing the above.
xsd:integeris unbounded and the lexer has long emitted a distinctBigIntegertoken pasti64; term position consumes it and lowering promotes it toFlakeValue::BigInt, buttry_parse_literalhad no arm for it, soFILTER(?v > 100000000000000000000)was a parse error while the same bound as a typed literal worked. Expressions parse-as the unary operator, so only the unsigned literal was missing. Parse-only: such bounds still have noRangeValueand do not push down.RangeValue::Decimalis added so decimal bounds push into the scan-sideObjectBoundslike integers and doubles. No encoding work was needed:ObjectBounds::matchesis already class-aware, and the scan's key-range narrowing only engages for temporal types.Performance
No plan changes for queries that pushed down correctly before. Mixed decimal filters now take the same in-scan lane as all-integer filters instead of the generic filter operator. Partially addresses #1335: decimal bounds now push into the scan-side
ObjectBounds, but the inline order-preserving decimal encoding and the seek-level key narrowing for numeric bounds both remain, and the issue stays open.Tests
And; pushdown consumer neither narrows nor consumes a partially-extractable filter.xsd:integerparses in expression position, unsigned and negated, andi64::MAXstill lexes asIntegerwhile one past it becomesBigInteger. The e2e table gained the three bare-form cases; under the reverted parser arm they fail with "Expected expression".RangeValue::Decimallanded, every literal in the case table became representable, so reverting the total-or-nothing arm left both e2e tests green and only the planner units caught it. Anxsd:integerbound pasti64has noRangeValueat all, so a conjunct carrying one still exercises the partial-extraction path; both directions are covered so a lane returning nothing cannot pass the empty case by accident. Re-verified under the reverted arm: both lanes fail on that case, returning all six rows instead of none.Not in scope
xsd:integerconstants beyondi64are still not pushed. The filter stays and is evaluated generically, so results are correct; making them push is part of feat: inline order-preserving xsd:decimal encoding + numeric range pushdown #1335.0.01as a double, so that surface never reached the decimal path; no twin test was needed.Gates
cargo fmt --check,cargo clippy --all --all-features --all-targets -D warnings,cargo nextest run --workspace --all-features: clean. The one workspace failure was the known LocalStackPortNotExposedtestcontainer flake, which passes isolated.mainis merged into the branch (clean). Re-run after the merge:cargo fmt --all --check,cargo test -p fluree-db-query --lib planner::(104 passed),cargo test -p fluree-db-api --test grp_query_sparql(374 passed). On the merged head every CI job was green:fmt,clippy,test,testsuite-sparql,wasm32,wasm-smoke,sql-bridge,bench-pathsandbench-compare.For the parser commit, re-run locally:
cargo test -p fluree-db-sparql(642 passed),cargo test -p fluree-db-api --test grp_query_sparql(374 passed), and the workspace-excludedtestsuite-sparqlW3C suite (81 passed, 0 failed) since this one touches the parser.cargo fmt --all --checkandcargo clippy -p fluree-db-sparql -p fluree-db-api --all-targets --no-depsclean.