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 ?-contained operator support #227

Merged
merged 4 commits into from Dec 20, 2014

Conversation

Projects
None yet
6 participants
@tminglei
Contributor

tminglei commented Dec 1, 2014

Resending for #187. Pls help review and merge. Thanks!

Currently it seems PgJDBC didn't support ?-contained operator escaping, which lead to some ?-contained operators, such as hstore's ?&/?| and ltree's ?/?~/?@ and so on, can't be used in PreparedStatement.

This pull request try to resolve the problem.

How to use it?
double mark the ? when passing a raw sql to connection's prepareStatement method, as below:

st = conn.prepareStatement("SELECT 'Top.Collections.Pictures.Astronomy.Stars'::ltree ?? '{\"*.Astronomy.*\"}'::lquery[];");

Then PgJDBC won't treat the ?? as parameter placeholders, and will restore it back to ? before sending to postgres server.

@ringerc

This comment has been minimized.

Member

ringerc commented Dec 1, 2014

Sorry to be a nag, but this should be documented in doc/pgjdbc.xml, probably under the escapes chapter (around line 1834) or under the ext chapter. I'll try to get to this if you don't.

@tminglei

This comment has been minimized.

Contributor

tminglei commented Dec 1, 2014

Hi @ringerc I just added the doc for ?-contained operator support. But, can you help double check it, since I'm not very familiar with the syntax.

Thanks for your nice help! :)

@tminglei

This comment has been minimized.

Contributor

tminglei commented Dec 18, 2014

Is this pull request acceptable?

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 18, 2014

Sorry I will open up some time to get to this shortly.

9.4 has been released today and I will attempt to get most if not all
significant PR's merged before I release the 9.4 driver

Dave Cramer

On 18 December 2014 at 09:34, 涂名雷 notifications@github.com wrote:

Is this pull request acceptable?


Reply to this email directly or view it on GitHub
#227 (comment).

@tminglei

This comment has been minimized.

Contributor

tminglei commented Dec 18, 2014

Well, so it missed the latest release?
In fact, there are many ?-contained operators I want to add support in slick-pg.

But any way thank you for your nice help! =)

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 18, 2014

No it has not missed the latest release. What I meant to say is I will find
time to push it in before the latest release

Dave Cramer

On 18 December 2014 at 09:54, 涂名雷 notifications@github.com wrote:

Well, so it missed the latest release?
In fact, there are many ?-contained operators I want to add support in
slick-pg https://github.com/tminglei/slick-pg.

But any way thank you for your nice help! =)


Reply to this email directly or view it on GitHub
#227 (comment).

@tminglei

This comment has been minimized.

Contributor

tminglei commented Dec 18, 2014

Glad to hear that! Thank you very much!! =)

@davecramer davecramer closed this Dec 19, 2014

@davecramer davecramer reopened this Dec 19, 2014

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 19, 2014

Hi, It would appear something in this pull broke the build. Can you look
into it please ?

Dave Cramer

On 18 December 2014 at 10:02, 涂名雷 notifications@github.com wrote:

Glad to hear that! Thank you very much!! =)


Reply to this email directly or view it on GitHub
#227 (comment).

@tminglei

This comment has been minimized.

Contributor

tminglei commented Dec 19, 2014

I don't know why, but it seems not related to my changes.
Can you help rebuild it?

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 19, 2014

I just initiated a build with your code and that is what is failing. Why do
you believe it is not your code ?

Dave Cramer

On 19 December 2014 at 06:25, 涂名雷 notifications@github.com wrote:

I don't know why, but it seems not related to my changes.
Can you help rebuild it?


Reply to this email directly or view it on GitHub
#227 (comment).

@tminglei

This comment has been minimized.

Contributor

tminglei commented Dec 19, 2014

As you know, my first commit built passed, and second commit built failed.
But the second commit is impossible to break the build.

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 19, 2014

I'm not sure I understand what 'impossible to break the build means' .
Other commits have been committed since your first commit. I initiated
another travis-ci to see if your code would still pass.

Currently it is not. I will have a quick look to see what the issue is.

Dave Cramer

On 19 December 2014 at 06:34, 涂名雷 notifications@github.com wrote:

As you know, my first commit built passed, and second commit built failed.
But the second commit is impossible to break the build.


Reply to this email directly or view it on GitHub
#227 (comment).

@tminglei

This comment has been minimized.

Contributor

tminglei commented Dec 19, 2014

Well, my second commit is just some word changes in doc/pgjdbc.xml, so it's impossible to break the build.
In fact, many error messages seems also not related to my changes.

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 19, 2014

OK, I have verified that Parser.unmarkDoubleQuestion is removing the second
' from "insert into test_a (imagename,image,id) values
('comttest',1234,5678)"

Dave Cramer

On 19 December 2014 at 06:55, 涂名雷 notifications@github.com wrote:

Well, my second commit is just some doc changes in doc/pgjdbc.xml, so
it's impossible to break the build.
In fact, many error messages seems also not related to my changes.


Reply to this email directly or view it on GitHub
#227 (comment).

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 19, 2014

This code

       j = Parser.parseSingleQuotes(aChars, i, standardConformingStrings

);

        buf.append(aChars, i, j-i);

        i = j;

seems to be the culprit, what I don't understand is how this passed before
and doesn't now?

Dave Cramer

On 19 December 2014 at 08:07, Dave Cramer davecramer@gmail.com wrote:

OK, I have verified that Parser.unmarkDoubleQuestion is removing the
second ' from "insert into test_a (imagename,image,id) values
('comttest',1234,5678)"

Dave Cramer

On 19 December 2014 at 06:55, 涂名雷 notifications@github.com wrote:

Well, my second commit is just some doc changes in doc/pgjdbc.xml, so
it's impossible to break the build.
In fact, many error messages seems also not related to my changes.


Reply to this email directly or view it on GitHub
#227 (comment).

@tminglei

This comment has been minimized.

Contributor

tminglei commented Dec 19, 2014

But this code was copied from pgjdbc itself. So strange?!

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 19, 2014

Stranger that it passed the first time and doesn't now

Dave Cramer

On 19 December 2014 at 08:56, 涂名雷 notifications@github.com wrote:

But this code was copied from pgjdbc itself. So strange?!


Reply to this email directly or view it on GitHub
#227 (comment).

@tminglei

This comment has been minimized.

Contributor

tminglei commented Dec 19, 2014

so, do you have any idea?

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 19, 2014

well I can fix the code by changing i-j to i-j+1 I am running the tests
now. I'd also like to see more test coverage specifically comments, etc.

Where did you copy this from in the code ?

Dave Cramer

On 19 December 2014 at 09:18, 涂名雷 notifications@github.com wrote:

so, do you have any idea?


Reply to this email directly or view it on GitHub
#227 (comment).

@tminglei

This comment has been minimized.

Contributor

tminglei commented Dec 19, 2014

Well, I think it was from V2Query, but I made some modification when creating method unmarkDoubleQuestion.

So it's obvious that I introduced the defect. Sorry!

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 19, 2014

Well I see the difference and why it is now failing. What I don't
understand is why it wasn't failing before.

the V2Query code just jumps over the single, double quotes and then
increments i

your code copies it in but fails to copy the last single or double quote.

Does your code currently pass tests on your machine ?

This is what I changed your code to:

switch (aChars[i])

        {

            case '\'': // single-quotes

                j = Parser.parseSingleQuotes(aChars, i,

standardConformingStrings);

                buf.append(aChars, i, j-i+1);

                i = j;

                break;


            case '"': // double-quotes

                j = Parser.parseDoubleQuotes(aChars, i) ;

                buf.append(aChars, i, j-i+1);

                i = j;

                break;


            case '-': // possibly -- style comment

                j = Parser.parseLineComment(aChars, i);

                buf.append(aChars, i, j-i+1);

                i = j;

                break;


            case '/': // possibly /* */ style comment

                j = Parser.parseBlockComment(aChars, i);

                buf.append(aChars, i, j-i+1);

                i = j;

                break;


            case '$': // possibly dollar quote start

                j = Parser.parseDollarQuotes(aChars, i);

                buf.append(aChars, i, j-i+1);

                i = j;

                break;

Dave Cramer

On 19 December 2014 at 09:39, 涂名雷 notifications@github.com wrote:

Well, I think it was from V2Query, but I made some modification when
creating method unmarkDoubleQuestion.


Reply to this email directly or view it on GitHub
#227 (comment).

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 19, 2014

Unfortunately this fails for me on the comments, and I have no more time
today.

I really want to see this get in the code, but at this point it's not ready
to be merged.

Dave Cramer

On 19 December 2014 at 10:00, Dave Cramer davecramer@gmail.com wrote:

Well I see the difference and why it is now failing. What I don't
understand is why it wasn't failing before.

the V2Query code just jumps over the single, double quotes and then
increments i

your code copies it in but fails to copy the last single or double quote.

Does your code currently pass tests on your machine ?

This is what I changed your code to:

switch (aChars[i])

        {

            case '\'': // single-quotes

                j = Parser.parseSingleQuotes(aChars, i,

standardConformingStrings);

                buf.append(aChars, i, j-i+1);

                i = j;

                break;


            case '"': // double-quotes

                j = Parser.parseDoubleQuotes(aChars, i) ;

                buf.append(aChars, i, j-i+1);

                i = j;

                break;


            case '-': // possibly -- style comment

                j = Parser.parseLineComment(aChars, i);

                buf.append(aChars, i, j-i+1);

                i = j;

                break;


            case '/': // possibly /* */ style comment

                j = Parser.parseBlockComment(aChars, i);

                buf.append(aChars, i, j-i+1);

                i = j;

                break;


            case '$': // possibly dollar quote start

                j = Parser.parseDollarQuotes(aChars, i);

                buf.append(aChars, i, j-i+1);

                i = j;

                break;

Dave Cramer

On 19 December 2014 at 09:39, 涂名雷 notifications@github.com wrote:

Well, I think it was from V2Query, but I made some modification when
creating method unmarkDoubleQuestion.


Reply to this email directly or view it on GitHub
#227 (comment).

@tminglei

This comment has been minimized.

Contributor

tminglei commented Dec 20, 2014

Ok, I'll check it agian. Sorry for the inconvenience to you!

@@ -133,8 +133,9 @@ else if (!isDollarQuoteContChar(query[d]))
public static int parseLineComment(final char[] query, int offset) {
if (offset + 1 < query.length && query[offset + 1] == '-')
{
while (++offset < query.length)

This comment has been minimized.

@tminglei

tminglei Dec 20, 2014

Contributor

Note: the offset will be set a value outside of array length when it's the last line

@tminglei

This comment has been minimized.

Contributor

tminglei commented Dec 20, 2014

Hi @davecramer resolved, pls help check again.
Again, sorry for the inconvenience to you!

For the cause, I think it's related to that I have different local environment, and maybe I also made some other mistakes.

@davecramer davecramer closed this Dec 20, 2014

@davecramer davecramer reopened this Dec 20, 2014

davecramer added a commit that referenced this pull request Dec 20, 2014

Merge pull request #227 from tminglei/contains-operator
add ?-contained operator support
Thanks for all your work on this!

@davecramer davecramer merged commit 31c5fcf into pgjdbc:master Dec 20, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@tminglei tminglei deleted the tminglei:contains-operator branch Dec 21, 2014

@lukaseder

This comment has been minimized.

lukaseder commented Jul 14, 2016

I'm curious about how this relates to the potential of there being a ?? operator...?

@vlsi

This comment has been minimized.

Member

vlsi commented Jul 14, 2016

I'm curious about how this relates to the potential of there being a ?? operator...?

  1. PostgreSQL core team is aware that ? has well defined meaning in JDBC, thus it is very unlikey for new operator to be named like ??

  2. Even if that operator existed, it could be called via prepareStatement("select 1 ???? 2") that is translated to select 1 ?? 2 when sending the query to the database. pgjdbc just converts every double ?? to a single literal ?, so every query text is possible provided you use enough question marks.

Does that answer your question?

@lukaseder

This comment has been minimized.

lukaseder commented Jul 14, 2016

Hmm, I think I have already seen such an operator somewhere (like ??# or something), but possibly that was a third-party extension.

The trouble I'm having with this escaping technique is the fact that JDBC code will no longer be executable in PostgreSQL directly. Besides, JDBC code now depends on whether a prepared statement or a static statement is used:

// Works today, with single question mark
c.createStatement().executeQuery("select '{}'::jsonb ?| array['a', 'b']");

// Will now require a double question mark for escaping
c.prepareStatement("select '{}'::jsonb ??| array['a', 'b']").executeQuery();

I just don't have a good feeling with this escaping of the question mark. Perhaps, I would have wished for the PostgreSQL JDBC driver to implement some smart parser logic to see if at any given location, a bind variable is even allowed. For instance, when writing select 1 ? 2, the ? can't really be a bind variable. Of course, that would require a bit of parsing logic...

The reason why I'm concerned is because this affects all upstream APIs (like jOOQ, JDBI, MyBatis, JPA, etc.). Specifically, if an upstream API allows for reading SQL strings from external sources (e.g. from templates), then the author of the external source will need to know that the statement will be processed with JDBC eventually...

@davecramer

This comment has been minimized.

Member

davecramer commented Jul 14, 2016

Hi Lukas,

Obviously this is not a great situation. However there really isn't a great
solution either. We (JDBC) do not really want to do full parsing of the
statement. PostgreSQL added the ? operator. We are stuck in this
situation... If you have a suggestion how to alleviate the pain, I'm all
ears.

Dave Cramer

On 14 July 2016 at 08:45, Lukas Eder notifications@github.com wrote:

Hmm, I think I have already seen such an operator somewhere (like ??# or
something), but possibly that was a third-party extension.

The trouble I'm having with this escaping technique is the fact that JDBC
code will no longer be executable in PostgreSQL directly. Besides, JDBC
code now depends on whether a prepared statement or a static statement is
used:

// Works today, with single question mark
c.createStatement().executeQuery("select '{}'::jsonb ?| array['a', 'b']");
// Will now require a double question mark for escaping
c.prepareStatement("select '{}'::jsonb ??| array['a', 'b']").executeQuery();

I just don't have a good feeling with this escaping of the question mark.
Perhaps, I would have wished for the PostgreSQL JDBC driver to implement
some smart parser logic to see if at any given location, a bind variable is
even allowed. For instance, when writing select 1 ? 2, the ? can't really
be a bind variable. Of course, that would require a bit of parsing logic...

The reason why I'm concerned is because this affects all upstream APIs
(like jOOQ, JDBI, MyBatis, JPA, etc.). Specifically, if an upstream API
allows for reading SQL strings from external sources (e.g. from templates),
then the author of the external source will need to know that the
statement will be processed with JDBC eventually...


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#227 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAYz9rqZr7rxnfhEPt07qehS6cwb0f_Rks5qVi98gaJpZM4DCVpS
.

@lukaseder

This comment has been minimized.

lukaseder commented Jul 14, 2016

I don't have a suggestion, but a workaround: http://stackoverflow.com/a/38370973/521799

Just use the corresponding jsonb_exists(jsonb, text), or exist(hstore, text) functions, instead :)

@polobo

This comment has been minimized.

polobo commented Jul 14, 2016

On Thu, Jul 14, 2016 at 8:54 AM, Lukas Eder notifications@github.com
wrote:

I don't have a suggestion, but a workaround:
http://stackoverflow.com/a/38370973/521799

Just use the corresponding jsonb_exists(jsonb, text), or exist(hstore,
text) functions, instead :)

​That works but is limited. Some parts of PostgreSQL require the use of an
operator to function. Indexes and the "ANY()" construct come to mind.

David J.​

@vlsi

This comment has been minimized.

Member

vlsi commented Jul 14, 2016

For instance, when writing select 1 ? 2, the ? can't really be a bind variable

I'm afraid, full or close to full parsing is required to tell if ? is bind or not.
Sooner or later someone will feed Buffalo buffalo Buffalo buffalo buffalo buffalo Buffalo buffalo into the parser and complain how beautifully it breaks.

When translating to SQL it might be ?- 2. Is it unary_op 2 or bind - 2?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment