Skip to content

Handle convertPatternInExpr. #1088

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

Merged
merged 12 commits into from
Sep 15, 2021
Merged

Conversation

zelch
Copy link
Contributor

@zelch zelch commented Jul 16, 2021

This is a second attempt at #976 , but now with support for Expr, List, and Not in addition to Sel.

This is used for queries like the following:

SELECT * from foo where field in (select substring(?, 1, seq) from seq_1_to_19);

SELECT a, b from foo where foo.a in (select a from bar where bar.b = ?);

SELECT a, b from foo where foo.a in (?, ?);

SELECT a, b from foo where foo.a not in (?, ?);

SELECT a, b from foo where ? in (foo.a, foo.b);

The support is heavily based on that found in the fork found at:
https://github.com/xiazemin/sqlc.git

However it should be noted that this is not just a straight copy and
paste job, there are several pieces from that branch for features that
don't exist in the main sqlc repository, the sqlc repository has changed
a bit since the fork, and the fork version does not handle the latter
case at all.

Zephaniah E. Loss-Cutler-Hull added 4 commits April 12, 2021 10:21
This can be handled just by calling convert on the subquery, and it is
necessary for some kinds of queries.

Like the following, without this we don't pickup the parameter, and thus
can't handle the query at all:

select * from foo where field in (select substring(?, 1, seq) from seq_1_to_19)
This is used for queries like the following:

SELECT a, b from foo where foo.a in (select a from bar where bar.b = ?);

SELECT a, b from foo where foo.a in (?, ?);

SELECT a, b from foo where foo.a not in (?, ?);

SELECT a, b from foo where ? in (foo.a, foo.b);

The support is heavily based on that found in the fork found at:
https://github.com/xiazemin/sqlc.git

However it should be noted that this is not just a straight copy and
paste job, there are several pieces from that branch for features that
don't exist in the main sqlc repository, the sqlc repository has changed
a bit since the fork, and the fork version does not handle the latter
case at all.
This is used for queries like the following:

SELECT a, b from foo where foo.a in (select a from bar where bar.b = ?);

SELECT a, b from foo where foo.a in (?, ?);

SELECT a, b from foo where foo.a not in (?, ?);

SELECT a, b from foo where ? in (foo.a, foo.b);

The support is heavily based on that found in the fork found at:
https://github.com/xiazemin/sqlc.git

However it should be noted that this is not just a straight copy and
paste job, there are several pieces from that branch for features that
don't exist in the main sqlc repository, the sqlc repository has changed
a bit since the fork, and the fork version does not handle the latter
case at all.
This covers all the cases I can think of, though if I missed some, it's
quite possible that they are broken in the code as well as absent in the
tests.
@zelch
Copy link
Contributor Author

zelch commented Jul 16, 2021

@asterikx pinging you on this one given the conversation on the previous merge request.

@zelch
Copy link
Contributor Author

zelch commented Jul 16, 2021

Well, oops. Please hold off on reviewing this, the code it generates won't compile, as noted in the pipeline checks.

I'll take a crack at resolving that sometime in the next week, I hope.

Zephaniah E. Loss-Cutler-Hull added 6 commits July 15, 2021 21:31
That is, we want to deduplicate the same column from showing up multiple
times, and thus we want to avoid adding a suffix, when we are getting
the results from a query, however we most definitely do not want to do
this for query parameters.

Especially since the logic for handling this in the query parameters is
missing the deduplication, but also because the database itself expects
the same number of parameters as we have placeholders in the query.
fieldName is after any mangilng to make it fit go variable standards,
and since that is the name that we are actually using, that is the name
that we should be checking for conflicts.

This only matters when we have multiple fields that differ only by
things that are changed when we are converting to the go variable
standards.
This should now build a little better, now that we have fixed the
generator.
Oops, this fixes a bug introduced in
885bd3d.
Needed now that we're merging in master.
@zelch
Copy link
Contributor Author

zelch commented Jul 16, 2021

Alright, so a bug in the golang code generator has been fixed, I'm not sure how easy it would have been to trigger this previously, but it is far too easy to trigger it with this change set.

And after merging the code with master, a logical conflict (lack of support for DefaultSchema in the pre-merge code) has been fixed.

At this point I think this should be good to go.

@kyleconroy
Copy link
Collaborator

@zelch This code is now out of date with main. Also, there are still no tests for PostgreSQL. Do you still have time to work on getting this across the line? If not, I'll close it out and work on IN support in a different PR.

@zelch
Copy link
Contributor Author

zelch commented Sep 13, 2021

I can definitely get it back up to date with main.

As far as Postgres tests, I'm afraid that I only implemented support for dolphin. Adding support for the postgresql engine would definitely be good to do, but I'm not sure that I'm going to have time in the immediate future to tackle it.

@kyleconroy
Copy link
Collaborator

Great, let's get it updated and then we can merge in support as-is.

Resolved a reasonably trivial merge conflict.
@zelch
Copy link
Contributor Author

zelch commented Sep 14, 2021

Done, and it is passing all tests.

@kyleconroy kyleconroy merged commit c8bf991 into sqlc-dev:main Sep 15, 2021
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.

2 participants