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

Allow mysql upserts #86

Closed
wants to merge 1 commit into from
Closed

Conversation

dball
Copy link

@dball dball commented Aug 23, 2015

While I have a bespoke version of this externally, it would be nice to
gracefully take advantage of honeysql's reader literals for sql fn calls
and the like.

While I have a bespoke version of this externally, it would be nice to
gracefully take advantage of honeysql's reader literals for sql fn calls
and the like.
@ddellacosta
Copy link
Collaborator

I'm not really convinced by this as it's very MySQL specific as far as I can tell--this is completely different syntax from the version that will land in PostgreSQL 9.5, for example, not to mention it's distinct from the merge syntax described in the SQL spec and used by other DBs.

This seems like a prime candidate for a MySQL specific helper lib or supplemental code within a specific codebase.

@dball
Copy link
Author

dball commented Aug 25, 2015

I don't think it's terribly different than postgresql's ON CONFLICT UPDATE clause? The :mode value would be :postgresql, the :updates value could remain the same, generating a series of SETs for a seq and DO NOTHING when not a seq, and you'd just need another value to specify the upsert key.

I don't use postgresql much, but I'd be happy to take a stab at generating such a thing. I think honeysql is more usable when such common features are well-supported in the core. I already did something along these lines when I added locking select support with database-specific modes.

@ddellacosta
Copy link
Collaborator

I maintain that it's a bad idea to start down the path of adding a bunch of vendor-specific extensions into honeysql. honeysql is designed the way it is to make it very easy to extend, and unless something is a part of the core SQL spec I disagree there's much value in incorporating it into the core library.

Again, I think the right place for this is either a MySQL-specific library of honeysql extensions or in your codebase itself--this is how things have been done up until now and I don't see why this should change.

That's my thinking-- @jkk @MichaelBlume (and anyone else) care to weigh in?

@dball
Copy link
Author

dball commented Aug 25, 2015

Two minor counter-arguments: there's already specific support for postgresql's parameters, and mysql and postgresql's locking select modes.

I am fairly sympathetic to the argument that if honeysql wants to support sql dialects, the dialect should be an argument to format, not an attribute of the query. I'd be happy to explore that option in a branch if y'all think that might be fruitful. Alternately, if the core project really intends to support only the core sql language, I'll also be happy to start up honey-mysql repo and call it a day.

@dball
Copy link
Author

dball commented Aug 25, 2015

(Btw, lest I come across as overly argumentative: I really appreciate your attention to these tickets and the project generally!)

@ddellacosta
Copy link
Collaborator

(I don't think you're being argumentative.)

I think that is a valid counterpoint but my argument would actually be that we should remove those and create some PostgreSQL- and MySQL-specific libs!

But at the very least I think these should be in their own namespace.

I'm not going to close this one out at this point or say anything definitive, I think it's worthwhile if at least @jkk especially can weigh in on this first, as this comes up a fair bit (and will most likely come up again if nothing is resolved). So I'll leave it at that for now.

@danielbraun
Copy link

@dball Thanks for the commit, using it in my code.
Maybe create a honeysql-contrib libray?

@dball
Copy link
Author

dball commented Jun 28, 2016

tbqh, my next rainy day project is going to be introducing a sql formatter into a fork, which would provide a home not only for vendor-specific query extensions, it would allow sql clause order to be manipulated without futzing with global state

@ddellacosta
Copy link
Collaborator

ddellacosta commented Jun 29, 2016

@dball would be interested in seeing that. Think it sounds like a potentially good move for the project...but I should add, would still love to see MySQL- and PostgreSQL- (and Oracle...etc.) specific vendor libs at some point, or something like that...

@dball
Copy link
Author

dball commented Jun 29, 2016

Sure, yeah, I agree. If such a thing were to come to pass, I would think honeysql would just have a generic ansi sql formatter, with vendor-specific formatters provided by add-in libs.

@arichiardi
Copy link
Contributor

My 2c, I really like the honeysql-postgres library approach, probably they could be under the same honeysql umbrella on Github so that folks see them at the same time listed there.

@seancorfield
Copy link
Owner

Given the precedence of honeysql-postgres, having a honeysql-mysql library with this sort of extension seems reasonable -- would you consider creating such a library?

@seancorfield seancorfield added the vendor-specific I think this belongs in a dialect, i.e., not the default behavior? label Jun 24, 2018
@dball
Copy link
Author

dball commented Jun 24, 2018

I haven't used mysql or honeysql in a few years, so don't want to commit the time. Would you prefer that I close out the issue or leave it open for future interested parties?

@seancorfield
Copy link
Owner

I'm inclined to close out all the vendor-specific issues and encourage folks who want those things to contribute to/create honeysql-mysql etc. Sounds like this issue can be closed without objection from the OP so I will. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vendor-specific I think this belongs in a dialect, i.e., not the default behavior?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants