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.

Sign up for GitHub

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

Jump to bottom

Add CONDITIONED BY * EXCEPT support #101

Merged
merged 4 commits into from
Apr 29, 2024
Merged

Add CONDITIONED BY * EXCEPT support #101

merged 4 commits into from
Apr 29, 2024

Conversation

KingMob
Copy link
Contributor

@KingMob KingMob commented Apr 12, 2024

No description provided.

@KingMob KingMob changed the base branch from main to push-rqxkyqrrsmrw April 12, 2024 15:37
@KingMob KingMob force-pushed the push-rqxkyqrrsmrw branch from 5a5786b to 909b81d Compare April 15, 2024 15:07
@KingMob KingMob force-pushed the push-lkxpwuxqvszx branch from d590a9a to 715478b Compare April 15, 2024 15:07
@codecov Codecov
Copy link

codecov bot commented Apr 15, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 75.72%. Comparing base (08be109) to head (ac9865a).

Files Patch % Lines
src/inferenceql/query/scalar.cljc 89.70% 5 Missing and 2 partials ⚠️
src/inferenceql/query/log.cljc 40.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #101      +/-   ##
==========================================
+ Coverage   75.60%   75.72%   +0.12%     
==========================================
  Files          30       30              
  Lines        1496     1516      +20     
  Branches       64       65       +1     
==========================================
+ Hits         1131     1148      +17     
- Misses        301      303       +2     
- Partials       64       65       +1

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

@KingMob KingMob force-pushed the push-rqxkyqrrsmrw branch from 909b81d to 013d3ed Compare April 15, 2024 15:23
@KingMob KingMob force-pushed the push-lkxpwuxqvszx branch from 715478b to 87e5225 Compare April 15, 2024 15:23
@KingMob KingMob marked this pull request as ready for review April 15, 2024 15:25
Base automatically changed from push-rqxkyqrrsmrw to main April 15, 2024 15:35
@KingMob KingMob force-pushed the push-lkxpwuxqvszx branch from 87e5225 to f4abb7c Compare April 15, 2024 15:37
@KingMob KingMob marked this pull request as draft April 15, 2024 15:53
@KingMob KingMob force-pushed the push-lkxpwuxqvszx branch from f4abb7c to ab09e62 Compare April 16, 2024 13:59
@KingMob KingMob marked this pull request as ready for review April 16, 2024 14:12
@KingMob KingMob force-pushed the push-lkxpwuxqvszx branch from 77bb484 to 2640e4a Compare April 17, 2024 14:16
@KingMob KingMob force-pushed the push-lkxpwuxqvszx branch 2 times, most recently from 125346b to 47d3e11 Compare April 26, 2024 10:25
@KingMob KingMob force-pushed the push-lkxpwuxqvszx branch from 47d3e11 to cf2af50 Compare April 26, 2024 15:20
Schaechtle
Schaechtle approved these changes Apr 29, 2024
View reviewed changes
Copy link
Contributor

@Schaechtle Schaechtle left a comment

Choose a reason for hiding this comment

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

Looks mostly good. Left a comment about a line I don't understand.

src/inferenceql/query/scalar.cljc

[:model-expr child] (plan child)
[:model-expr "(" child ")"] (plan child)
[(:or :conditioned-by-expr :conditioned-by-except-clause) & _] (conditioned-by-plan* ws-free-node)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure what's going on here. Why is this phrase starting with an :or? Maybe add a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a comment.

In core.match, (:or :alternative1 :alternative2) means it could match either of two alternatives in that spot.

I did that to match either subpart, and delegate further matching/planning to conditioned-by-plan*.

And I started delegating to submatching fns because once the core.match patterns got too big/nested, the Clojure compiler started generating invalid Java class names. See https://clojure.atlassian.net/browse/CLJ-1852 and https://ask.clojure.org/index.php/3824/clojure-generated-class-names-length-exceed-file-system-limit?show=3824#q3824 for more.

src/inferenceql/query/scalar.cljc Outdated Show resolved Hide resolved
@KingMob KingMob force-pushed the push-lkxpwuxqvszx branch from cf2af50 to ac9865a Compare April 29, 2024 15:54
@KingMob KingMob merged commit 680e377 into main Apr 29, 2024
7 checks passed
@KingMob KingMob deleted the push-lkxpwuxqvszx branch April 29, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Schaechtle Schaechtle Schaechtle approved these changes

@zane zane Awaiting requested review from zane

@sritchie sritchie Awaiting requested review from sritchie

@littleredcomputer littleredcomputer Awaiting requested review from littleredcomputer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KingMob @Schaechtle