Skip to content

Commit 1131ae6

Browse files
authored
fix(query): NOT/predicate/or-join binding regressions + adopt-vector >32 corruption (#816)
* fix(query): defer NOT/predicate/or-join checks to align with iterative resolver The legacy Relation engine's `-resolve-clause*` cases for predicates, NOT, NOT-JOIN, and OR-JOIN-with-required-vars all used `check-all-bound` / `check-some-bound` to RAISE eagerly when their input vars weren't yet bound. But the iterative resolver in `datahike.tools/resolve-clauses` already supports clause deferral via nil-return — `bind-by-fn` uses it, and the resolver re-queues failed clauses for the next pass until a fixed point is reached. The asymmetry meant that a perfectly resolvable query could be rejected when a NOT/predicate sat after a binder chain that itself needed the resolver's retry pass. Concretely: [(get-else $ ?e :v :__null__) ?vv] ; deferred until ?e bound [(get-else $ ?e :w :__null__) ?ww] [(?fn ?vv ?ww) ?v1] ; deferred until ?vv ?ww bound (not [(contains? #{…} ?v1)]) ; eagerly raised on ?v1 unbound [?e :marker true] ; would have bound ?e Fixed by switching the four eager call sites to `(when (X-bound? …) …)` defer patterns, mirroring `bind-by-fn`. When a query is genuinely unsolvable, the resolver still raises — now with the more accurate "Cannot resolve any more clauses" listing every unresolvable clause, instead of a single misleading "Insufficient bindings" pointing at whichever clause happened to be checked first. Side fix: `update-ctx-with-stats` now propagates `nil` from `update-fn` (silently kept a half-built map otherwise — only triggered for stats-on queries that go through a deferring clause). Tests: - New `test-deferred-clause-binding` regression test in query_not_test covering predicate, NOT, fn-call→NOT chain, and or-join. - Existing `test-insufficient-bindings` (and the attribute-refs mirror) used to assert the buggy eager-raise; updated to the new behavior that matches the compiled engine. - Existing `test-clause-order` legacy-engine branch was dead weight (codified the bug); replaced with a single both-engines assertion. All 1444 unit tests + 6 integration tests pass. * fix(query/lower,plan): NOT-binding validator must read function ops' :binding The plan-builder's post-ordering NOT-binding check (lower.cljc Step 7 and the equivalent loop in plan.cljc create-plan) walked ordered ops to build a vars-so-far set, but used `(:bind-vars op)` for `:function` ops — a key plan-function-op never sets. The op contributed nothing, so any subsequent NOT/predicate whose only required var came from a function chain was falsely rejected with "Insufficient bindings". Concretely, the Odoo `_check_removed_columns` query goes through pgwire-datahike with `*force-legacy* false` (planner enabled). Its SQL `format_type(a.atttypid, a.atttypmod) NOT IN (...)` becomes: [(get-else $ ?a_eid :pg_attribute/atttypid :__null__) ?atttypid] [(get-else $ ?a_eid :pg_attribute/atttypmod :__null__) ?atttypmod] [(?fmt ?atttypid ?atttypmod) ?v1] (not [(contains? #{...} ?v1)]) After ordering, the planner walked the ops correctly but the function-op contribution was dropped, leaving ?v1 invisible to the NOT validation. Same op-cost code at plan.cljc:790 uses :args for inputs and is fine — only the vars-so-far accumulator was broken. Fixed by reading `(:binding op)` (the canonical key from plan-function-op) and passing through `analyze/extract-vars` so scalar / tuple / list / map binding forms all work uniformly. Pgwire-datahike's failing Odoo query now succeeds end-to-end on the planner path. Verified directly: parsed SQL → q-result with both *force-legacy* true (legacy engine, was already fixed in prior commit) and false (planner path, this commit's fix). All 1444 tests pass on both engines; 6983 assertions on legacy, 7010 on planner. * fix(query/execute): adopt-vector silently corrupted >32-element rows `clojure.lang.PersistentVector/adopt` is a zero-copy wrapper that builds the vector with `cnt = arr.length`, `shift = 5`, `root = EMPTY_NODE`, and the data in `tail`. That layout is only valid for vectors of length ≤ 32 (the entire body fits in tail; tailoff = 0). For longer arrays adopt silently produces a corrupt vector: cnt > 32 implies tailoff = cnt-32 > 0, but root remains EMPTY_NODE, so any indexed access at i < tailoff walks `EMPTY_NODE.array` and dies with NPE: Cannot read field "array" because "node" is null The corruption isn't surfaced until a `seq`/`nth`/`take`/`subvec` runs — at which point the call site looks completely innocent (just `(seq row)` on a `clojure.lang.PersistentVector`). Hard to spot in code review. Surfaced in production by pgwire-datahike running Odoo's `_auto_init` field-reflection: `SELECT 34 cols FROM res_partner WHERE id IN (1)` materialised one row whose backing Object[] had length 35 (34 user columns + ?eid for :with). pgwire's `format-query-result` hidden-strip step `(vec (take visible row))` then NPE'd on the first chunkedSeq access. The runtime check is essentially free: `LazilyPersistentVector /createOwning` does the right dispatch — `adopt` for arr.length ≤ 32 (unchanged hot path), and `PersistentVector/create` (transient-build, valid tree) for longer arrays. Note: the bug has been latent since `adopt-vector` was added — every query whose materialised tuple width exceeded 32 was at risk. Most queries don't hit that arity, which is why no existing test caught it. Tests: - New `test-find-arity-greater-than-32` regression in query_test: 35-element :find with 35-attr schema, asserts seq/nth/take all succeed and round-trip through (vec (take k row)) cleanly. - All 1444 unit tests still pass with DATAHIKE_QUERY_PLANNER=true. * Fix format.
1 parent 164b073 commit 1131ae6

10 files changed

Lines changed: 245 additions & 118 deletions

File tree

src/datahike/query.cljc

Lines changed: 55 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1227,9 +1227,22 @@
12271227
:rels (->> (:rels context)
12281228
(keep #(limit-rel % vars)))))
12291229

1230+
(defn- ctx-bound-vars [context]
1231+
(set (concat (mapcat #(keys (:attrs %)) (:rels context))
1232+
(keys (:consts context)))))
1233+
1234+
(defn all-bound?
1235+
"True iff every var in `vars` is currently bound in `context`."
1236+
[context vars]
1237+
(set/subset? (set vars) (ctx-bound-vars context)))
1238+
1239+
(defn some-bound?
1240+
"True iff at least one var in `vars` is currently bound in `context`."
1241+
[context vars]
1242+
(boolean (seq (set/intersection (set vars) (ctx-bound-vars context)))))
1243+
12301244
(defn check-all-bound [context vars form]
1231-
(let [bound (set (concat (mapcat #(keys (:attrs %)) (:rels context))
1232-
(keys (:consts context))))]
1245+
(let [bound (ctx-bound-vars context)]
12331246
(when-not (set/subset? vars bound)
12341247
(let [missing (set/difference (set vars) bound)]
12351248
(log/raise "Insufficient bindings: " missing " not bound in " form
@@ -1238,12 +1251,10 @@
12381251
:vars missing})))))
12391252

12401253
(defn check-some-bound [context vars form]
1241-
(let [bound (set (concat (mapcat #(keys (:attrs %)) (:rels context))
1242-
(keys (:consts context))))]
1243-
(when (empty? (set/intersection vars bound))
1244-
(log/raise "Insufficient bindings: none of " vars " is bound in " form
1245-
{:error :query/where
1246-
:form form}))))
1254+
(when (empty? (set/intersection vars (ctx-bound-vars context)))
1255+
(log/raise "Insufficient bindings: none of " vars " is bound in " form
1256+
{:error :query/where
1257+
:form form})))
12471258

12481259
(defn resolve-context [context clauses]
12491260
(dt/resolve-clauses resolve-clause context clauses))
@@ -1894,8 +1905,14 @@
18941905
([context clause orig-clause]
18951906
(condp looks-like? clause
18961907
[[symbol? '*]] ;; predicate [(pred ?a ?b ?c)]
1897-
(do (check-all-bound context (identity (filter free-var? (first clause))) orig-clause)
1898-
(filter-by-pred context clause))
1908+
;; Defer if any input var isn't bound yet — the iterative resolver
1909+
;; (datahike.tools/resolve-clauses) will retry once binders fire.
1910+
;; If the var is never bound, the resolver raises "Cannot resolve any
1911+
;; more clauses" with the full pending list, which is more useful
1912+
;; than a misleading single-clause error from this site.
1913+
(let [vars (filter free-var? (first clause))]
1914+
(when (all-bound? context vars)
1915+
(filter-by-pred context clause)))
18991916

19001917
[[symbol? '*] '_] ;; function [(fn ?a ?b) ?res]
19011918
(bind-by-fn context clause)
@@ -1918,8 +1935,8 @@
19181935

19191936
'[or-join [[*] *] *] ;; (or-join [[req-vars] vars] ...)
19201937
(let [[_ [req-vars & vars] & branches] clause]
1921-
(check-all-bound context req-vars orig-clause)
1922-
(recur context (list* 'or-join (concat req-vars vars) branches) clause))
1938+
(when (all-bound? context req-vars)
1939+
(recur context (list* 'or-join (concat req-vars vars) branches) clause)))
19231940

19241941
'[or-join [*] *] ;; (or-join [vars] ...)
19251942
;; TODO required vars
@@ -1953,34 +1970,34 @@
19531970

19541971
'[not *] ;; (not ...)
19551972
(let [[_ & clauses] clause
1956-
negation-vars (collect-vars clauses)
1957-
_ (check-some-bound context negation-vars orig-clause)
1958-
join-rel (reduce hash-join (:rels context))
1959-
negation-context (-> context
1960-
(assoc :rels [join-rel])
1961-
(assoc :stats [])
1962-
(resolve-context clauses))
1963-
negation-join-rel (reduce hash-join (:rels negation-context))
1964-
negation (subtract-rel join-rel negation-join-rel)]
1965-
(cond-> (assoc context :rels [negation])
1966-
(:stats context) (assoc :tmp-stats {:type :not
1967-
:branches (:stats negation-context)})))
1973+
negation-vars (collect-vars clauses)]
1974+
(when (some-bound? context negation-vars)
1975+
(let [join-rel (reduce hash-join (:rels context))
1976+
negation-context (-> context
1977+
(assoc :rels [join-rel])
1978+
(assoc :stats [])
1979+
(resolve-context clauses))
1980+
negation-join-rel (reduce hash-join (:rels negation-context))
1981+
negation (subtract-rel join-rel negation-join-rel)]
1982+
(cond-> (assoc context :rels [negation])
1983+
(:stats context) (assoc :tmp-stats {:type :not
1984+
:branches (:stats negation-context)})))))
19681985

19691986
'[not-join [*] *] ;; (not-join [vars] ...)
1970-
(let [[_ vars & clauses] clause
1971-
_ (check-all-bound context vars orig-clause)
1972-
join-rel (reduce hash-join (:rels context))
1973-
negation-context (-> context
1974-
(assoc :rels [join-rel])
1975-
(assoc :stats [])
1976-
(limit-context vars)
1977-
(resolve-context clauses)
1978-
(limit-context vars))
1979-
negation-join-rel (reduce hash-join (:rels negation-context))
1980-
negation (subtract-rel join-rel negation-join-rel)]
1981-
(cond-> (assoc context :rels [negation])
1982-
(:stats context) (assoc :tmp-stats {:type :not
1983-
:branches (:stats negation-context)})))
1987+
(let [[_ vars & clauses] clause]
1988+
(when (all-bound? context vars)
1989+
(let [join-rel (reduce hash-join (:rels context))
1990+
negation-context (-> context
1991+
(assoc :rels [join-rel])
1992+
(assoc :stats [])
1993+
(limit-context vars)
1994+
(resolve-context clauses)
1995+
(limit-context vars))
1996+
negation-join-rel (reduce hash-join (:rels negation-context))
1997+
negation (subtract-rel join-rel negation-join-rel)]
1998+
(cond-> (assoc context :rels [negation])
1999+
(:stats context) (assoc :tmp-stats {:type :not
2000+
:branches (:stats negation-context)})))))
19842001

19852002
'[*] ;; pattern
19862003
(let [source rel/*implicit-source*

src/datahike/query/execute.cljc

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -246,9 +246,27 @@
246246
d)))))))))
247247

248248
(defn- adopt-vector
249-
"Create a PersistentVector from an object array without copying."
249+
"Create a PersistentVector from an object array.
250+
251+
`clojure.lang.PersistentVector/adopt` is fast (zero-copy) but only
252+
correct when `arr.length <= 32`. It constructs the vector with
253+
`root = EMPTY_NODE` and the data in the tail, which is the
254+
PersistentVector internal layout for short vectors. For arrays
255+
longer than 32 the result is silently corrupt: `cnt > 32` but
256+
`tailoff() = cnt-32 > 0`, and any `arrayFor(i)` for i < tailoff
257+
walks `EMPTY_NODE.array` and NPEs on the first level.
258+
259+
Real-world repro: SELECT against a 33+-column table from pgwire
260+
(Odoo's res_partner has 34 columns). The corrupt row crashes at
261+
the first `seq`/`nth`/`take` with `Cannot read field \"array\"
262+
because \"node\" is null`.
263+
264+
`LazilyPersistentVector/createOwning` does the right dispatch:
265+
the cheap adopt for length ≤ 32, and `PersistentVector/create`
266+
(transient-build, valid tree) for longer arrays. Still no copy
267+
in the short path."
250268
[^objects arr]
251-
#?(:clj (clojure.lang.PersistentVector/adopt arr)
269+
#?(:clj (clojure.lang.LazilyPersistentVector/createOwning arr)
252270
:cljs (vec arr)))
253271

254272
;; ---------------------------------------------------------------------------

src/datahike/query/lower.cljc

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -407,7 +407,24 @@
407407
(filterv #(#{:entity-group :pattern-scan} (:op %)) ordered-ops))
408408

409409
;; ---------------------------------------------------------------
410-
;; Step 7: NOT binding validation
410+
;; Step 7: NOT binding validation.
411+
;; Walks the ordered ops in execution order, tracking which vars
412+
;; are bound after each op runs. NOT/NOT-JOIN must have at least
413+
;; one of its vars bound by a prior op (legacy semantics).
414+
;;
415+
;; The per-op contribution-set must mirror what the executor
416+
;; will actually bind:
417+
;; - :entity-group → scan + merge vars
418+
;; - :pattern-scan → pattern vars
419+
;; - :function → the result-binding var (`:binding` from
420+
;; plan-function-op). Predicates produce no
421+
;; new bindings; or/or-join handle their own
422+
;; binding internally.
423+
;; Earlier this case used `(:bind-vars op)` which plan-function-op
424+
;; never sets — function ops looked like they bound nothing, so
425+
;; any subsequent NOT/predicate whose only required var came from
426+
;; a function chain (e.g. `format_type(...)` feeding NOT IN) was
427+
;; falsely rejected with "Insufficient bindings".
411428
_ (loop [remaining ordered-ops
412429
vars-so-far bound-vars]
413430
(when (seq remaining)
@@ -425,7 +442,7 @@
425442
:entity-group (into (:vars (:scan-op op))
426443
(mapcat :vars (:merge-ops op)))
427444
:pattern-scan (:vars op)
428-
:function (into #{} (filter analyze/free-var?) (:bind-vars op))
445+
:function (analyze/extract-vars (:binding op))
429446
nil))))))]
430447

431448
{:ops ordered-ops

src/datahike/query/plan.cljc

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1397,7 +1397,15 @@
13971397
:entity-group (into (:vars (:scan-op op))
13981398
(mapcat :vars (:merge-ops op)))
13991399
:pattern-scan (:vars op)
1400-
:function (into #{} (filter analyze/free-var?) (:bind-vars op))
1400+
;; plan-function-op stores the result var(s) in
1401+
;; :binding (scalar, tuple, list, or map). The
1402+
;; legacy `:bind-vars` key is never set —
1403+
;; reading it lost the result-var contribution
1404+
;; and falsely tripped the NOT validation when
1405+
;; a function-chain output was the only var
1406+
;; reaching a NOT clause. Mirror lower.cljc's
1407+
;; identical loop.
1408+
:function (analyze/extract-vars (:binding op))
14011409
nil))))))]
14021410

14031411
{:ops ordered-ops

src/datahike/query_stats.cljc

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,17 +17,22 @@
1717
(:rels context))})
1818

1919
(defn update-ctx-with-stats
20-
"update-fn must expect [context] as argument"
20+
"update-fn must expect [context] as argument.
21+
Returns nil when update-fn returns nil — that is the iterative
22+
resolver's defer signal (datahike.tools/resolve-clauses re-queues
23+
the clause for the next pass). Without the nil propagation, stats
24+
collection would silently keep a half-built map and confuse retries."
2125
[context clause update-fn]
2226
(if (:stats context)
23-
(let [{:keys [res t]} (dt/timed #(update-fn context))
24-
clause-stats (merge (get-stats res)
25-
{:clause clause
26-
:t t}
27-
(:tmp-stats res))]
28-
(-> res
29-
(update :stats conj clause-stats)
30-
(dissoc :tmp-stats)))
27+
(let [{:keys [res t]} (dt/timed #(update-fn context))]
28+
(when res
29+
(let [clause-stats (merge (get-stats res)
30+
{:clause clause
31+
:t t}
32+
(:tmp-stats res))]
33+
(-> res
34+
(update :stats conj clause-stats)
35+
(dissoc :tmp-stats)))))
3136
(update-fn context)))
3237

3338
(defn extend-stat

test/datahike/test/attribute_refs/query_not_test.cljc

Lines changed: 13 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -176,31 +176,19 @@
176176
(shift-in #{[4 3] [3 3] [4 4]} [0 1] ref-e0)))
177177

178178
(deftest test-insufficient-bindings
179-
(if datahike.test.core-test/compiled-engine?
180-
;; Compiled engine reorders NOT after its bindings — reorderable cases are valid queries
181-
(do
182-
(testing "reorderable NOT — compiled engine handles correctly"
183-
(is (set? (d/q '[:find ?e :where (not [?e :mname "Ivan"]) [?e :mname]] test-db))))
184-
(testing "NOT-JOIN with inner vars bound within body"
185-
(is (set? (d/q '[:find ?e :where [?e :mname]
186-
(not-join [?e] (not [1 :age ?a]) [?e :age ?a])]
187-
test-db)))))
188-
;; Legacy engine requires bindings before NOT
189-
(are [q msg] (thrown-with-msg? Throwable msg
190-
(d/q (into '[:find ?e :where] q)
191-
test-db))
192-
'[(not [?e :mname "Ivan"])
193-
[?e :mname]]
194-
#"Insufficient bindings: none of #\{\?e\} is bound"
195-
196-
'[[?e :mname]
197-
(not-join [?e]
198-
(not [1 :age ?a])
199-
[?e :age ?a])]
200-
#"Insufficient bindings: none of #\{\?a\} is bound"))
201-
202-
;; Both engines: truly unbound vars must throw
179+
;; Both engines now accept NOT before its binder — see
180+
;; datahike.test.query-not-test for the rationale (legacy engine's
181+
;; iterative resolver defers and retries NOT/predicate clauses).
182+
(testing "reorderable NOT — both engines handle correctly"
183+
(is (set? (d/q '[:find ?e :where (not [?e :mname "Ivan"]) [?e :mname]] test-db))))
184+
(testing "NOT-JOIN with inner vars bound within body"
185+
(is (set? (d/q '[:find ?e :where [?e :mname]
186+
(not-join [?e] (not [1 :age ?a]) [?e :age ?a])]
187+
test-db))))
188+
189+
;; Truly unbound vars still error — message changes from
190+
;; "Insufficient bindings" to "Cannot resolve any more clauses".
203191
(testing "truly unbound vars throw"
204-
(is (thrown-with-msg? Throwable #"Insufficient bindings"
192+
(is (thrown-with-msg? Throwable #"Cannot resolve any more clauses|Insufficient bindings"
205193
(d/q '[:find ?e :where [?e :mname] (not [?a :mname "Ivan"])]
206194
test-db)))))

test/datahike/test/attribute_refs/query_or_test.cljc

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,9 @@
130130
[?e :age ?a])]
131131
test-db)))
132132

133-
(is (thrown-with-msg? Throwable #"Insufficient bindings: #\{\?e\} not bound"
133+
;; or-join required-vars now defers; if no clause binds them, the
134+
;; iterative resolver raises "Cannot resolve any more clauses".
135+
(is (thrown-with-msg? Throwable #"Cannot resolve any more clauses|Insufficient bindings"
134136
(d/q '[:find ?e
135137
:where (or-join [[?e]]
136138
[?e :weight 40])]

0 commit comments

Comments
 (0)