FFFF fix(or): join an or sharing no variable with earlier clauses as a cross product (#610) by adityamukho · Pull Request #612 · project-minigraf/minigraf · GitHub
[go: up one dir, main page]

Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
8000
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed

- **Rule queries with a literal argument return every row** (#608). Magic-sets rewriting restricts a rule to the facts its callers demand, but it took each rule's bound arguments from the first call it saw and assumed every other call matched. A call that did not — the same rule called with free arguments elsewhere in the query, a call inside `or` or `not`, a call from a rule the query calls with free arguments, or a rule-body call whose argument nothing bound yet — either failed with `INT-022` (`(chain ?x :c)` with `(chain ?x ?y) :- (link ?x ?m) (chain ?m ?y)`) or silently returned too few rows (`(link ?x :c) (link ?p ?q)` returned only the `:c` edges for `?p ?q`). A query with both arguments literal next to another rule call also lost its rows. The demand on each rule is now the one every call site supplies, and a rule some call site reads in full is evaluated in full. Present since v1.2.0.
- **An `or` that shares no variable with the earlier clauses returns its cross product** (#610). Such an `or` — `[?x :edge ?z] (or [?p :edge :b] [?p :edge :c])` — returned no rows when no branch had `not`, `not-join` or a pseudo-attribute, because the incoming rows were replaced by the branch results. Each incoming row now joins every branch row, in queries and rule bodies. `or-join` still refuses a join variable no earlier clause binds (INT-018): it filters, it does not bind.
- **A prepared query keeps a literal `:as-of`, `:valid-at` or `:any-valid-time`** (#603). Binding a prepared query's slots dropped any `:as-of` or `:valid-at` that was not itself a slot, so `PreparedQuery::execute` and `PreparedQuery::query` ran at the current transaction time and valid now: `(query [:find ?v :as-of 1 :where [:a :n ?v]])` prepared returned the current rows, not those at transaction 1, and a prepared per-fact pseudo-attribute query with `:any-valid-time` failed with `INT-026`. Slot forms (`:as-of $tx`, `:valid-at $t`) were not affected. Present since 2026-04-04; fixed on v2.x too.
- **The wasm32 builds are tested again, and the browser layer no longer panics or drops pages.** The repo split (#231) moved the browser and WASI CI jobs to minigraf-wasm, which only builds the package, so core's own wasm tests stopped running and the wasm32 test build broke unnoticed. A new `wasm.yml` workflow runs the `wasm-bindgen` tests in headless Firefox and Chrome, runs the unit and integration tests on `wasm32-wasip1` under Wasmtime, and lints both targets with `clippy -D warnings`. Fixes found on the way: `BrowserDb` skipped any dirty page it could not read when flushing to IndexedDB and still reported success, and now returns the error (also fixed in v2.0.4, #470). The IndexedDB upgrade callback, the IndexedDB backend handle and the page-buffer lock no longer `unwrap()` (on wasm32 a panic aborts the whole instance). IndexedDB keys that are not whole page numbers are rejected instead of truncated. The nine `BrowserBufferBackend` tests used `#[test]`, which the wasm test runner never runs, and now run. File-only modules (`wal`, `storage::verify`, `storage::dir_sync`) are no longer compiled for wasm32, so the wasm32 builds have no warnings.
- **`BrowserDb` stores upgraded pages and keeps only live pages** (#411, #440). `BrowserDb.open()` now writes the pages an open produced to IndexedDB, so a v7 store is upgraded to v8 once instead of in memory on every open. Pages on the free list no longer keep their bytes in memory or their keys in IndexedDB: open drops them (after a v7 upgrade, the whole old v7 region), and each `transact`, `retract`, `checkpoint()` and `importGraph()` drops the pages its commit freed, in the same IndexedDB transaction as the commit's pages and meta page. Memory and IndexedDB size now follow the live data rather than the write history, and opening no longer reads dead pages back. `importGraph()` also deletes the keys of the replaced database's pages, which a reopen used to load next to the imported ones. `exportGraph()` writes free pages as zeros and is as long as the file's meta page says; the blob opens natively as before.
Expand Down
2 changes: 1 addition & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ Auto-migrates v7 → v8 on open (spec §9, with a backup meta page). v1–v6 are

## Test Coverage

**1548 tests** (1538 passing, 10 ignored; unit + integration + doc).
**1554 tests** (1544 passing, 10 ignored; unit + integration + doc).
See `docs/TEST_COVERAGE.md` for the full per-file breakdown.

**Testing conventions** — see the Testing Conventions section below before writing any tests.
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ let r2 = pq.execute(&[("tx", BindValue::TxCount(2)), ("entity", BindValue::Entit

```bash
cargo run # interactive Datalog REPL
cargo test # run 1548 tests
cargo test # run 1554 tests
cargo run < demos/demo_recursive.txt # recursive rules demo
```

Expand Down
5 changes: 3 additions & 2 deletions docs/TEST_COVERAGE.md

Large diffs are not rendered by default.

16 changes: 2 additions & 14 deletions src/query/datalog/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2252,20 +2252,8 @@ pub(crate) fn apply_or_clauses(
.filter(|v| !v.starts_with("__") && branch_var_names.contains(v.as_str()))
.collect();

if shared_vars.is_empty() {
// No shared user-visible variables between incoming bindings and branch
// results. This is semantically equivalent to a cross-join, which only
// makes sense when `bindings` carries no meaningful state yet (i.e. it
// is a single empty binding — the query start state — or all incoming
// variables are fact-metadata keys that the branch does not reference).
// In that case the cross-join degenerates to "replace with branch union",
// which is what the original seeded evaluation produced. Incoming bindings
// that carry user-visible variables but none matching any branch variable
// would be silently dropped here — that situation should not arise given
// that `or` must share at least one variable with the surrounding clause.
bindings = union_bindings;
continue;
}
// With no shared variable every key is empty, so each incoming
// row joins every branch row: a cross product (#610).

// Build HashMap: shared-key tuple → Vec<branch Binding>
let mut branch_map: HashMap<Vec<(String, Value)>, Vec<Binding>> = HashMap::new();
Expand Down
84 changes: 84 additions & 0 deletions tests/disjunction_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -670,3 +670,87 @@ fn test_rule_with_or_join_body_no_matches_returns_empty() {
.expect("rule with or-join and no colored entities must not error");
assert_eq!(result_count(&r), 0, "no colored entities");
}

fn edge_db() -> Minigraf {
let db = db();
db.execute(r#"(transact [[:a :edge :b] [:b :edge :c]])"#)
.unwrap();
db
}

/// #610: an `or` sharing no variable with the earlier clauses joins as a cross product.
#[test]
fn test_or_with_no_shared_variable_is_cross_product() {
let db = edge_db();
let r = db
.execute(r#"(query [:find ?x ?p :where [?x :edge ?z] (or [?p :edge :b] [?p :edge :c])])"#)
.unwrap();
assert_eq!(result_count(&r), 4, "?x in {{a, b}} times ?p in {{a, b}}");

// Clause order does not change the answer.
let r = db
.execute(r#"(query [:find ?x ?p :where (or [?p :edge :b] [?p :edge :c]) [?x :edge ?z]])"#)
.unwrap();
assert_eq!(result_count(&r), 4, "or first: same cross product");
}

/// #610: the cross product keeps the earlier clauses' variables.
#[test]
fn test_or_cross_product_keeps_incoming_variables() {
let db = edge_db();
let r = db
.execute(r#"(query [:find ?z ?p :where [?x :edge ?z] (or [?p :edge :c])])"#)
.unwrap();
assert_eq!(result_count(&r), 2, "two ?z values, one ?p value");
}

/// #610: an `or` whose branches match nothing still empties the answer.
#[test]
fn test_or_with_no_shared_variable_and_no_match_is_empty() {
let db = edge_db();
let r = db
.execute(r#"(query [:find ?x ?p :where [?x :edge ?z] (or [?p :edge :zz] [?p :edge :yy])])"#)
.unwrap();
assert_eq!(result_count(&r), 0, "no branch matches");
}

/// #610: the seeded path (a branch with `not`) gives the same cross product.
#[test]
fn test_or_with_not_branch_and_no_shared_variable_is_cross_product() {
let db = edge_db();
let r = db
.execute(
r#"(query [:find ?x ?p
:where [?x :edge ?z]
(or (and [?p :edge :b] (not [?p :edge :zz]))
[?p :edge :c])])"#,
)
.unwrap();
assert_eq!(result_count(&r), 4, "seeded path: cross product");
}

/// #610: the same `or` in a rule body.
#[test]
fn test_rule_with_or_and_no_shared_variable_is_cross_product() {
let db = edge_db();
db.execute(r#"(rule [(pair ?x ?p) [?x :edge ?z] (or [?p :edge :b] [?p :edge :c])])"#)
.unwrap();
let r = db
.execute(r#"(query [:find ?x ?p :where (pair ?x ?p)])"#)
.unwrap();
assert_eq!(result_count(&r), 4, "rule body: cross product");
}

/// #610: incoming rows carrying fact metadata (a pseudo-attribute) still join.
#[test]
fn test_or_cross_product_after_pseudo_attribute() {
let db = edge_db();
let r = db
.execute(
r#"(query [:find ?x ?t ?p :any-valid-time
:where [?x :edge ?z] [?x :db/tx-count ?t]
(or [?p :edge :c])])"#,
)
.unwrap();
assert_eq!(result_count(&r), 2, "two edges, one tx-count each, one ?p");
}
Loading
0