Skip to content

Conversation

@zonotope
Copy link
Contributor

@zonotope zonotope commented Jan 5, 2026

This patch extends the existing in-line filter optimization for single variable filter functions to multi-variable functions. It also introduces a clause-oriented query reordering flow.

Filters are separated from binding patterns at the beginning of the optimization step, the binding patterns are then reordered, and then both single- and multi- variable filters are inlined onto the sorted binding patterns when safe. The same separation, reordering, and inlining now occur within nested clauses, improving performance while preserving correctness and determinism.

Filters are inlined onto the sorted list of binding patterns at the point when exactly one referenced variable becomes newly bound by a pattern, and all other referenced variables were bound by a previous pattern. Filters that reference multiple variables and would have more than one newly bound in the same pattern are not inlined; they are emitted immediately after that pattern as a (not inline) :filter pattern.

@zonotope zonotope requested a review from a team January 5, 2026 02:59
@bplatz
Copy link
Contributor

bplatz commented Jan 8, 2026

The optimizer tests :exists, :not-exists, and :minus as if they bind vars that escape to the outer solution, so it could incorrectly push a pending top-level filter into those clauses based on “inner bindings”. -- the following test will fail which demonstrate this. Here is a sample query:

{:select  '[?name ?age]
 :where   '[[:exists [{:type :ex/User :schema/age ?age}]]
            {:type :ex/User, :schema/age ?age, :schema/name ?name}
            [:filter "(> ?age 45)"]]}

Assume DB has at least one user with :schema/age > 45 (e.g. Brian=50), and also users with age ≤ 45 (Alice=42, Cam=34).

  • Correct result: only users with ?age > 45 (e.g. Brian 50, David 46)
  • Wrong result (after bad push into :exists): :exists becomes globally true (because someone has age > 45), and then you get all users back (including Alice/Cam) because the top-level filter is gone.

This can be fixed by not counting bindings inside :exists/:not-exists/:minus as guaranteed outer bindings. Filter pushdown rules should, I think, should be:

  • :exists / :not-exists: only push a filter in if all its vars are already bound in the outer scope.
  • :minus: never push filters in (it evaluates without outer bindings).

I think adding pattern bindings for exists, not-exists and minus to return #{} in optimize.cljc and then apply the pushdown rules above will work.

(deftest exists-should-not-claim-bindings-escape
  (testing "a filter that depends on a var bound later must not be pushed into :exists just because inner clause binds that var name"
    ;; Vars introduced inside :exists do not escape to the outer solution.
    ;; Pushing a top-level filter into :exists can change results.
    (let [inner-exists    [(vector (where/unmatched-var '?x)
                                   (where/unmatched-var '?p)
                                   (where/unmatched-var '?age))]
          exists-pattern  (where/->pattern :exists inner-exists)
          outer-binds-age (vector (where/unmatched-var '?s)
                                  (where/unmatched-var '?p2)
                                  (where/unmatched-var '?age))
          filter-desc     {:fn    (constantly true)
                           :forms ['(> ?age 45)]
                           :vars  #{'?age}}
          {:keys [patterns filters]}
          (opt/nest-filters [exists-pattern outer-binds-age] [filter-desc] #{})]
      (is (= [exists-pattern outer-binds-age] patterns)
          "nest-filters should not rewrite by pushing the filter into :exists here")
      (is (= [filter-desc] filters)
          "filter should remain top-level (pending) until ?age is actually bound in the outer clause"))))

Copy link
Contributor

@bplatz bplatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good other than the minus, et.c issues I brought up...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants