korma / Korma

Tasty SQL for Clojure.
http://sqlkorma.com
1.48k stars 222 forks source link

Union queries do not work on SQLite at all #328

Open jvah opened 8 years ago

jvah commented 8 years ago

Union queries are not working because of extra parenthesis, to reproduce I used following dependencies in project.clj:

[org.clojure/java.jdbc "0.4.2"]
[org.xerial/sqlite-jdbc "3.8.11.2"]
[korma "0.4.2"]

And then in REPL:

=> (require '[korma.db :as db])
=> (require '[korma.core :refer :all])
=> (db/defdb sqlite (db/sqlite3 {:db "test.db"}))
=> (defentity users)
=> (defentity admins)
=> (-> (union*) (queries (subselect users) (subselect admins)) (exec))
SQLException [SQLITE_ERROR] SQL error or missing database (near "(": syntax error)  org.sqlite.core.DB.newSQLException (DB.java:890)
=> (-> (union*) (queries (subselect users) (subselect admins)) (as-sql))
"(SELECT \"users\".* FROM \"users\") UNION (SELECT \"admins\".* FROM \"admins\")"

To try the same in sqlite3 console:

sqlite> (SELECT "users".* FROM "users") UNION (SELECT "admins".* FROM "admins"); 
Error: near "(": syntax error
sqlite> SELECT "users".* FROM "users" UNION SELECT "admins".* FROM "admins"; 
Error: no such table: admins

I've tried all kinds of tricks, but haven't been able to get rid of the parenthesis without modifying korma code. My understanding is that the SQL standard requires the version without parenthesis to work, but does not specify behaviour with the parenthesis. I don't think the reason of Microsoft Access wanting parenthesis applies here, since its documentation doesn't use parenthesis either.

The relevant parts in the code are engine.clj rows 372-375, where sql-combination-query calls map-val for each subquery, and map-val calls utils/wrap for each subquery, which in turn adds the parenthesis. I'm just wondering what is the correct way of fixing this. Could just remove utils/wrap from map-val, but this might have some side-effects in other queries, or alternatively could not call map-val and instead concat the bound-params manually and return the query string. Any suggestions which sounds better?

immoh commented 8 years ago

I don't think utils/wrap can be removed from map-val as it is needed for IN subqueries. There has to be some other way to have different handling for union/union all/intersect subqueries.

juhovh commented 8 years ago

Sorry for the account confusion, I'm working this partly from work partly on free time. :)

Anyway, added a pull request that fixes the problem and updates tests accordingly. I ended up duplicating the map-val functionality into a separate similar function. The fallback to pred-map is not strictly necessary, but I thought it might be ok to be 1:1 with the map-vals in this respect. As far as I know union/union all/intersect can only work with subqueries, but I'm happy to be proven wrong.

venantius commented 6 years ago

@juhovh It doesn't like you ever actually opened a pull request for your branch.

I'm going through old issues and trying to clean them up. Is this still a priority?

If there hasn't been a response to this issue in 2 weeks, I'll close the ticket.