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

Add support for MySQL’s LIMIT with optional offset argument. #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

temochka
Copy link
Contributor

MySQL supports a LIMIT statement with an optional offset argument, i.e. LIMIT 10, 20 which stands for "fetch 20 records after skipping the first 10".

Noticed this while working on #18. I didn’t want to couple these changes, since I’m not sure if you’re interested in supporting such extensions of SQL standard. If you are, I can help with merging these.

@r0man
Copy link
Owner

r0man commented Aug 14, 2014

If this can be done with offset, I don't think we need to
change this. I use this library mostly with PostgreSQL and the
unwritten plan is to support features of that dialect.

I'm not sure if I want to get into the business of adding vendor
specific SQL (other than PostgreSQL) at this point. I think this
needs some thought, proper tests against real databases and a
design change in the compiler.

That said, I'm happy to make changes like the LIMIT ALL that
allows people to run this against other databases.

Roman

@temochka
Copy link
Contributor Author

I agree that it looks like it could be solved by adding more polymorphism into compiler, i.e. dispatch based on platform’s supported features (GCC back-end probably does something similar). However it’s hard to find the right balance and not introduce some sort of linter/optimizer for SQL syntax while going in that direction (i.e. replacing LIMIT 0,1 with LIMIT 1 OFFSET 0). Personally I’d like to see sqlingvo as a lightweight DSL that allows people to build and compose SQL queries while delegating validation to a target database. Considering this, it’s still not clear to me how to transparently support platform-specific extensions like MySQL’s LIMIT X,Y. I’m not even mentioning GROUP_CONCAT(DISTINCT column_name ORDER BY column_name DESC SEPARATOR ',') (oh, wait, I am) :-)

I’m totally fine with not merging this pull request, just interested in your opinion about this.

@r0man
Copy link
Owner

r0man commented Aug 14, 2014

Hi Artem,

you can try this:

(require '[sqlingvo.compiler :refer [defarity compile-whitespace-args]])
(defarity compile-whitespace-args
  "group-concat")

(select [:student-name '(group-concat DISTINCT :test-score ORDER BY :test-score DESC SEPARATOR " ")]
  (from :student)
  (group-by :student-name))
;=> ["SELECT \"student_name\", group-concat(DISTINCT \"test_score\" ORDER BY \"test_score\" DESC SEPARATOR ?) FROM \"student\" GROUP BY \"student_name\"" " "]

Regarding platform specific extensions? The only thing I had in
mind is dispatching not only on :op, but also on the database
vendor, providing a default implementation for most of the stuff
and let oeople override vendor specifc compilation ops.

Since I'm a happy PostgreSQL user and don't have to deal with
anything else at the moment, this is not very high priority at
the moment ...

If you have a better solution, I'm all ears.

Roman

@r0man
Copy link
Owner

r0man commented Aug 14, 2014

I think something like this for dispatching
http://stackoverflow.com/questions/9440253/defmethod-catch-all

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

2 participants