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

Add GIVEN * EXCEPT ... support #103

Merged
merged 2 commits into from
Apr 30, 2024
Merged

Add GIVEN * EXCEPT ... support #103

merged 2 commits into from
Apr 30, 2024

Conversation

KingMob
Copy link
Contributor

@KingMob KingMob commented Apr 17, 2024

No description provided.

@KingMob KingMob marked this pull request as ready for review April 17, 2024 13:53
Copy link

codecov bot commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 75.91%. Comparing base (680e377) to head (62930fc).

Files Patch % Lines
src/inferenceql/query/permissive.cljc 91.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
+ Coverage   75.72%   75.91%   +0.18%     
==========================================
  Files          30       30              
  Lines        1516     1536      +20     
  Branches       65       66       +1     
==========================================
+ Hits         1148     1166      +18     
- Misses        303      304       +1     
- Partials       65       66       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KingMob KingMob force-pushed the push-rmqnwwwmmlps branch from abdb4bc to b7908a0 Compare April 17, 2024 14:16
@KingMob KingMob force-pushed the push-lkxpwuxqvszx branch from 77bb484 to 2640e4a Compare April 17, 2024 14:16
Comment on lines +59 to +81
(defn ^:private ands->commas
"Replaces subsequent pairs of whitespace nodes followed by \"AND\" with
a comma."
[node]
;; Really awkward to do this with a loop, but reduce/partition alternatives
;; have the problem of not being able to look ahead easily, or skip over
;; elements. An index is actually easier.
(let [cnt (count node)
stop-idx (dec cnt)]
(if (= 1 cnt)
node
(loop [i 0
acc []]
(cond (> i stop-idx) acc ; final pair was ws+and, so we overshot
(= i stop-idx) (conj acc (peek node)) ; conj last elt
:else (let [n1 (nth node i)
n2 (nth node (inc i))]
(if (and (tree/whitespace? n1)
(string? n2)
(re-matches #"(?i)AND" n2))
(recur (+ 2 i) (conj acc ",")) ; skip to after AND
(recur (inc i) (conj acc n1)))))))))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If people think this is overkill, I can change or remove it. (Only real consequence is spaces before commas when unparsing the converted perm->strict query. E.g., model GIVEN * EXCEPT foo AND bar AND moop -> model CONDITIONED BY * EXCEPT foo , bar , moop.)

I know it's ugly, but the need to look at pairs, and skip over input makes it awkward. I considered variations using partition, reduce, rest/butlast, etc. and none of them turned out cleaner. Too many fns aren't designed to skip input or work with more than one item at a time.

However, I was able to come up with a recursive core.match version, though, if people prefer that. Only down side is it's 50% slower.

(defn ands->commas2
  "Like the above, but uses core.match recursively to look at two items at a time."
  [node]
  (match/match [node]
    [([n1 n2 & more] :seq)]
    (if (and (tree/whitespace? n1)
             (string? n2)
             (re-matches #"(?i)AND" n2))
      (cons "," (ands->commas2 more))
      (cons n1 (ands->commas2 (cons n2 more))))

    :else node))

I also tried using into [] and vectors, but lists are faster for building piece-wise.

@KingMob KingMob force-pushed the push-rmqnwwwmmlps branch from b7908a0 to f8bff4b Compare April 17, 2024 15:18
@KingMob KingMob force-pushed the push-lkxpwuxqvszx branch from 2640e4a to 125346b Compare April 26, 2024 10:20
@KingMob KingMob force-pushed the push-rmqnwwwmmlps branch from f8bff4b to 6ebfca9 Compare April 26, 2024 10:20
@KingMob KingMob force-pushed the push-lkxpwuxqvszx branch from 125346b to 47d3e11 Compare April 26, 2024 10:25
@KingMob KingMob force-pushed the push-rmqnwwwmmlps branch from 6ebfca9 to 0110d7e Compare April 26, 2024 10:25
@KingMob KingMob force-pushed the push-lkxpwuxqvszx branch from 47d3e11 to cf2af50 Compare April 26, 2024 15:20
@KingMob KingMob force-pushed the push-rmqnwwwmmlps branch 2 times, most recently from 52a0fc3 to 1529fde Compare April 29, 2024 15:54
@KingMob KingMob force-pushed the push-lkxpwuxqvszx branch from cf2af50 to ac9865a Compare April 29, 2024 15:54
Base automatically changed from push-lkxpwuxqvszx to main April 29, 2024 16:17
@KingMob KingMob force-pushed the push-rmqnwwwmmlps branch from 1529fde to 62930fc Compare April 29, 2024 16:33
Copy link
Contributor

@zane zane 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!

@@ -83,6 +109,7 @@
(defn strict-node
[node]
(match/match [(vec (remove tree/whitespace? node))]

Copy link
Contributor

Choose a reason for hiding this comment

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

Intentional? Fine either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The blank line? Uhh, don't recall. But with I do tend to add blank lines when the body of something has pairs of lines, like long conditionals in cond.

@KingMob KingMob merged commit 8041a2f into main Apr 30, 2024
7 checks passed
@KingMob KingMob deleted the push-rmqnwwwmmlps branch April 30, 2024 09:28
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