LauJensen / clojureql

ClojureQL is superior SQL integration for Clojure
https://clojureql.sabrecms.com
Eclipse Public License 1.0
285 stars 39 forks source link

Compile Nil to NULL #20

Closed r0man closed 13 years ago

r0man commented 13 years ago

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 commented 13 years ago

hmm. it should be:

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

right?

r0man commented 13 years ago

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

r0man commented 13 years ago

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 commented 13 years ago

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 commented 13 years ago

Hi Roman,

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

r0man commented 13 years ago

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 commented 13 years ago

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 commented 13 years ago

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