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

Compile Nil to NULL #20

Merged
2 commits merged into from Nov 20, 2010
Merged

Compile Nil to NULL #20

2 commits merged into from Nov 20, 2010

Conversation

r0man
Copy link
Collaborator

@r0man r0man commented Nov 19, 2010

Hi Lau & Ninjudd,

I added a small fix to handle nil in the compile-expr function. Before that change the following form

(-> (table {} :users) (select (where (= :id nil))) to-sql)

was generating

SELECT users.* FROM users WHERE (id = )

Now it will generate

SELECT users.* FROM users WHERE (id = NULL)

Roman.

@ninjudd
Copy link
Collaborator

ninjudd commented Nov 19, 2010

hmm. it should be:

SELECT users.* FROM users WHERE (id IS NULL)

right?

@r0man
Copy link
Collaborator Author

r0man commented Nov 19, 2010

Ahh, I'm in PostgreSQL land over here and it works for me, but you are right. I'll look into this tomorrow ...

@r0man
Copy link
Collaborator Author

r0man commented Nov 20, 2010

Added fix to compile nil values like this:

(compile-expr [:eq :id nil])
; => "(id IS NULL)"

(compile-expr [:eq nil :id])
; => "(id IS NULL)"

What should happen with this one?

(compile-expr [:eq nil nil])
; => "(NULL IS NULL)"

Should this return the above, true, 1 or something else?

@LauJensen
Copy link
Owner

Hi Roman,

Thanks for pitching in!

I've reviewed your commits and I have two questions.

  1. Why does the test suite now fail? When you commit please ensure that the tests run perfectly and that the README.md is not out of sync with your changes.

  2. You changed a very simple compiler which uses case to achieve constant time dispatch into a multimethod, why do you prefer this solution?

Thanks,
Lau

@LauJensen
Copy link
Owner

Hi Roman,

Disregard question 1, I had an uncommited change in my master which contaminated your branch, so sorry!

@r0man
Copy link
Collaborator Author

r0man commented Nov 20, 2010

Hi Lau,

regarding your 2nd question. When I started I put the code into the
defn as well. Later on I thought about my postgis stuff where I am
using the bounding box operator && and how I could build on top of
clojureql ...

With the multi method approach I can extend clojueql from the outside
by defining my own && operator like this:

(defmethod compile-expr :&& [expr]
   ... handle bounding box sql)

That's why I changed it into a multi method. Do you think it's a huge
performance loss?

Roman

@LauJensen
Copy link
Owner

The performance loss should be seen in the context of database queries, so its nearly non existant. My main concern was actually the loss of clarity and I wanted to understand your motivation for designing it the way you did.

Being able to plug in compiler extensions is something which could prove absolute central to ClojureQLs adoption on various backends so its something I really want designed right in the first go. You've given me some food for thought, so let me think it over and I'll get back to you.

Thanks for explaining,
Lau

@LauJensen
Copy link
Owner

r0man,

I've thought it over and I like this approach, it makes it simple to plug in your own predicate compile and the loss of speed/clarity isn't enough to outweight the benefits.

I've merged your changes so lets take them out for a spin and see how it goes.

Thanks alot for the patch!
Lau

This pull request was closed.
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.

None yet

3 participants