Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix lots of nested ands and ors #989

Merged
merged 5 commits into from
Mar 6, 2025
Merged

Conversation

dwwoelfel
Copy link
Contributor

Ran into this bug while working on cel -> instaql, where we can generate lots of nested ORs.

Three fixes in this PR:

  1. If there's a single AND or OR, then we just strip it out ([{:or [{handle: 'dww'}]}] is the same as {handle: 'dww'})
  2. Collapse the ors properly, so that [{:or [{:or [cond-a, cond-b]}, cond-c]]}] generates {:or [cond-a, cond-b, cond-c]} instead of {:or [[cond-a, cond-b], cond-c]}.
  3. Fix in datalog.clj to handle ORs in the destination path. I simplified the logic to only do recursion for generating the join conds in one place.

There is still a remaining issue where we could potentially include the same cte twice, leading to postgres complaints about ambiguous columns. I can't reproduce it without disabling the or collapsing, so I'm ignoring it for now. I think it might have something to do with how we generate join syms for the different groups of ors and ands. We might need to increment the level?

[{:expected 'non-empty-list?
:in (conj (:in state) :or)
:message "The list of `or` conditions can't be empty."}])))}
(let [conds (->> v
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is still overly complicated--I think it would be a lot easier to reason about if we got rid of the spec coercion, but that's a bigger project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oo, that could be interesting! Would make it nicer too when we print stuff deep inside the codebase

Copy link

github-actions bot commented Mar 6, 2025

Copy link
Contributor

@stopachka stopachka left a comment

Choose a reason for hiding this comment

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

Niice!

@dwwoelfel dwwoelfel merged commit 94a9a49 into main Mar 6, 2025
29 checks passed
@dwwoelfel dwwoelfel deleted the fix-lots-of-nested-ands-and-ors branch March 6, 2025 18:15
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.

2 participants