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

PARSE-QUERY: separate single-quote quoting from dollar quoting in parser #945

Open
wants to merge 1 commit into
base: master
from

Conversation

@phoe
Copy link
Collaborator

@phoe phoe commented Apr 22, 2019

Attempts to fix #944

I do not know how to run the pgloader unit/regression test suite - please tell me how to do it and/or verify that my patch does not break anything

TODO: Insert a regression test in the automated test suite that ensures that the following two queries are read properly.

DO $$ BEGIN
  -- ';'
END; $$ LANGUAGE plpgsql;
DO $A$ BEGIN
  DO $B$ BEGIN
    -- ';'
  END; $B$ LANGUAGE plpgsql;
END; $A$ LANGUAGE plpgsql;
@phoe
Copy link
Collaborator Author

@phoe phoe commented Apr 22, 2019

The following still fails to parse even after my PR:

SELECT '$$ DO $$';
@phoe
Copy link
Collaborator Author

@phoe phoe commented Apr 22, 2019

The new commit seems to fix the issue.

CL-USER> (with-input-from-string (s "SELECT '$$ $$';")
           (pgloader.sql::parse-query s))
"SELECT '$$ $$'"

CL-USER> (with-input-from-string (s "DO $$ BEGIN
        -- ';'
END; $$ LANGUAGE plpgsql;")
           (pgloader.sql::parse-query s))
"DO $$ BEGIN
        -- ';'
END; $$ LANGUAGE plpgsql"

CL-USER> (with-input-from-string (s "DO $A$ BEGIN
  DO $B$ BEGIN
    -- ';'
  END; $B$ LANGUAGE plpgsql;
END; $A$ LANGUAGE plpgsql;")
           (pgloader.sql::parse-query s))
"DO $A$ BEGIN
  DO $B$ BEGIN
    -- ';'
  END; $B$ LANGUAGE plpgsql;
END; $A$ LANGUAGE plpgsql"
@dimitri
Copy link
Owner

@dimitri dimitri commented Apr 24, 2019

Your current PR includes a FASL file it seems, of course I won't include it in the final merge. Also, you mentioned on IRC wanting to add unit testing to the SQL parser. This PR might be a nice place to do that, what do you think?

@phoe
Copy link
Collaborator Author

@phoe phoe commented Apr 24, 2019

Welp - I should not have committed the FASL file. I will skip it.

Sure - do you have any examples of SQL syntax that should be declined by this parser? I will do my best to prepare such myself, but external examples will also be handy.

@phoe
Copy link
Collaborator Author

@phoe phoe commented Feb 20, 2020

There - PR cleaned up. Sorry for the massive delay and the duplicate - I have realized that I've already mentioned (and fixed!) this issue only after I've submitted the duplicate.

As for tests for this, @sabracrolleton has created a series of tests for the version we've pulled into postmodern at https://github.com/marijnh/Postmodern/tree/master/postmodern/tests - you should be able to incorporate them back into pgloader.

Fix for SELECT '$$ $$'. Fixes #944.
@dimitri
Copy link
Owner

@dimitri dimitri commented Feb 29, 2020

Hi @phoe ; did you figure out how to run make check or make check-saved to pass the unit tests locally? They depend only on having a local Postgres instance, you don't need fancy source support such as SQLite or MySQL, though it's good practice to also have those handy and manually run some of the test cases for them.

Are you interested in contributor access to this repository and merging your PR, maybe with more tests if you feel like it? At the moment the make check test suite does not contain unit tests, only integration tests for things that are supposed to be working... So having a manual way to tests things from the SLIME REPL would be considered good enough here, really.

@phoe
Copy link
Collaborator Author

@phoe phoe commented Feb 29, 2020

Hey - I haven't had the time to figure that out. Maybe next week I will find some time and set up a pgloader development environment on my machine, though, so let's keep our hopes up.

Once I do that, I'll come back to this issue, and possibly try to import Sabra's tests and integrate it into pgloader's unit test suite.

@dimitri
Copy link
Owner

@dimitri dimitri commented Apr 3, 2020

Ping? anything we should still do about this one?

@phoe phoe self-assigned this Apr 3, 2020
@phoe
Copy link
Collaborator Author

@phoe phoe commented Apr 3, 2020

Yes - I still haven't written tests for that one. I'll assign myself to it for the time being and poke it in spare time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.