Fork me on GitHub
#honeysql
<
2021-07-17
>
Daniel Hugh19:07:41

👋 Hi, I am using honeysql version 2.0.0-rc3 and seem to have encountered a couple bugs with the :fetch clause. 1.

clj꞉user꞉> 
(require '[honey.sql :as sql])
nil
clj꞉user꞉> 
(sql/format {:select [:id :name]
             :from [:table]
             :offset 20 :fetch 10})
["SELECT id, name FROM table OFFSET ? FETCH ?" 20 10]
According to the second example in https://github.com/seancorfield/honeysql/blob/develop/doc/clause-reference.md#limit-offset-fetch we are missing the ONLY term
;; expected return
["SELECT id, name FROM table OFFSET ? FETCH ? ONLY" 20 10]
2.
clj꞉user꞉> 
(require '[honey.sql.helpers :as h])
nil
clj꞉user꞉> 
(h/fetch 10)
{:offset 10}
clj꞉user꞉> 
Should the map returned contain the :fetch key instead of the :offset key?

seancorfield19:07:37

@daniel_hugh Is it actually syntactically illegal for the JDBC driver you're using?

Daniel Hugh21:07:33

Yes it appears to be illegal. I am using a JDBC driver for Microsoft SQL Server and it looks like it requires the ONLY term according to the syntax docs https://docs.microsoft.com/en-us/sql/t-sql/queries/select-order-by-clause-transact-sql?view=sql-server-ver15 EDIT: Actually in my case, FETCH ? ONLY isn't enough 😞 I would need something like FETCH NEXT 20 ROWS ONLY with terms like NEXT and ROWS like described in this old issue https://github.com/seancorfield/honeysql/issues/58 So it looks like I will need to create a custom clause.

seancorfield23:07:32

Darn, I thought bits were optional. Sigh. I suspect OFFSET also needs ROW or ROWS for SQL Server? I'll have to do that based on dialect because MySQL can't have ROWS there. PostgreSQL can have OFFSET n or OFFSET n ROWS (of course it can!). And it can either have LIMIT n (like MySQL) or FETCH FIRST n ROWS ONLY (FIRST/NEXT are synonyms, ROW/ROWS are synonyms).

Daniel Hugh00:07:35

Yep seems like that is the case for OFFSET in SQL Server. What an interesting standard we have for SQL haha

seancorfield00:07:53

Are you specifying :sqlserver as your dialect, BTW?

seancorfield00:07:39

I'm going to set it up so if there's :offset and :fetch it will use this verbose style, if there's :offset and :limit it will use the abbreviated style, and if there's :offset on its own, it will only use the verbose style for the :sqlserver dialect -- which I think is the only way to not introduce breaking behavior.

Daniel Hugh00:07:18

No i am not currently using the dialect option

Daniel Hugh01:07:21

I think that will work for my use case. The query I have uses both :offset and :fetch

3
seancorfield01:07:59

@daniel_hugh OK, 2.0.0-rc5 is available on Clojars. Can you try it out and let me know if it works for you?

seancorfield01:07:15

Once I get the all-clear from you, I'll go ahead and actually announce that RC 🙂

Daniel Hugh01:07:05

Sure thing. Let me get on a computer

Daniel Hugh01:07:04

Great! That works for me 🙂 Thanks for implementing this so quickly 😄

seancorfield01:07:34

Thank you for catching this before I got to "gold"!

👍 3
Daniel Hugh01:07:11

Oh yeah, I suppose the original example that started this all would need to be updated as well with the verbose style. https://github.com/seancorfield/honeysql/blob/develop/doc/clause-reference.md#limit-offset-fetch

(sql/format {:select [:id :name]
             :from [:table]
             :offset 20 :fetch 10})

["SELECT id, name FROM table OFFSET ? ROWS FETCH NEXT ? ROWS ONLY" 20 10]

seancorfield01:07:28

Yeah, I'll be polishing the docs a lot over the next two weeks. I'll get that updated this evening (although it won't make the cljdoc version until the "gold" release).

👍 3
seancorfield01:07:57

OK, that section is updated. Please take a look and let me know if there are any improvements you can think of...

Daniel Hugh02:07:37

Looks good. I don't have any suggestions at the moment, but will let you know if anything comes up 🙂

seancorfield19:07:44

It's definitely a bug. I don't have a test for it, apparently 😞

Daniel Hugh22:07:07

By the way, I am not not sure if you caught the second issue in my original post about the fetch helper.

clj꞉user꞉> 
(require '[honey.sql.helpers :as h])
nil
clj꞉user꞉> 
(h/fetch 10)
{:offset 10}
clj꞉user꞉> 
Should the map returned contain the `:fetch` key instead of the `:offset` key?

seancorfield23:07:29

I did not see that, no, sorry.

seancorfield23:07:21

OK, that's fixed on develop.

👍 3