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

sqlite basic insert support #1653

Closed
wants to merge 1 commit into from

Conversation

javiercbk
Copy link
Contributor

I'm adding BASIC insert support for sqlite.

New feature for sqlite:

-- name: InsertAll :exec
INSERT INTO foo (a, b) VALUES (?, ?);

Previously sqlc is unable to generate inserts for sqlite because it was simply returning ast.TODO.

I'm implementing a basic support for inserts although "some gymnastics" are required to work with the parser.

The main problem is that the parser gives a list of expressions so, take the following example:

-- name: InsertAll :exec
INSERT INTO foo (a, b) VALUES (?, ?), (?, ?);

In this case the parser would give you a slice of 4 bind expression, the ONLY way to know how to generate the ast.List required is to have the columns explicit in the query.

Currently, it seems impossible to generate the correct instruction for the following sql

-- name: InsertAll :exec
INSERT INTO foo VALUES (?, ?), (?, ?);

In this case, we are screwed. Values are not "grouped" in any meaningful way. This can be solved by a quick "hack" but I really didn't dive deep in the parser as well to go with the hack approach. This is the reason why we are forced to explicitly put the columns on the insert. You would get a parser error if you want to do the query above although sqlite has no problem with this type of update.

I want to support INSERT with SELECT but that would be another kind of beast to tame. In the mean time, basic INSERT support is all that I needed in my project.

I'm open to comments, suggestions, questions and concerns.

@javiercbk javiercbk force-pushed the sqlite-basic-insert branch from 6c971f3 to 3194b8e Compare May 31, 2022 01:49
@DBarney
Copy link

DBarney commented Jun 16, 2022

@javiercbk I would like to help out on getting at least a little bit better sqlite support for sqlc. I cloned your branch and am running it locally and it does work, at least for simple things. For anything that I find that I can fix, would you like me to make a PR to your fork of the sqlc repo?

@russellhaering
Copy link

@javiercbk this PR seems pretty valuable even if it doesn't support omitting the column names. From what I can tell the tests are failing because of a version mismatch in the testdata file headers. Want to fix that and see if we can get Kyle to merge this?

@DBarney I'm about to push up a small PR with some improvements to SQlite + SELECT. I'm just working through things in the order I run into use-cases, but maybe its worth collaborating to make sure we don't duplicate work?

@DBarney
Copy link

DBarney commented Jun 17, 2022

@russellhaering that sounds good to me. And that’s basically what I’m doing too. I have a set of Postgresql queries that I am switching to sqlite, and as I run into issues I was going to fix them add them to the PR.

@hakobera
Copy link
Contributor

@javiercbk @russellhaering I just noticed I already did duplicate work in #1687

I checked quickly and found my PR covers both this PR (INSERT) and @russellhaering's PR SELECT ... WHERE ... LIMIT #1686

What do you think?

@javiercbk javiercbk force-pushed the sqlite-basic-insert branch from 3194b8e to 1623935 Compare June 17, 2022 19:29
@javiercbk
Copy link
Contributor Author

I'm going to fix that failing test right now. I think I've commited something I was working on by accident (insert with select and some values)

@javiercbk
Copy link
Contributor Author

@hakobera dude, I've just seen your comment...if your PR covers all that, then by all means lets go with your solution.

@javiercbk javiercbk closed this Jun 17, 2022
@javiercbk
Copy link
Contributor Author

@hakobera I'll checkout your PR and will do some testing. Using your work I would try to implement some missing operations

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.

4 participants