Fixes #30

[butters16]

Admittedly, the placeholders are a bit wonky, but the best way I could think to not touch the iterator logic.

[butters16]

@ilopmar Can you please let me know if this is likely to be pulled? My company is going to work from a local fork in the meantime, but it our obvious long-term goal is to get back to this repo. Also, it would be nice to know whether the solution is feasible in the eyes of the maintainers. If there are flaws, I'd be willing to try again. Thanks in advance.

[ilopmar]

Sorry to taking so long in responding. Are this changes backwards compatible or this breaks the compatibility?
Honestly since we added support for json and specially for jsonb I've never used hstore anymore. I have everything I need with jsonb.

[butters16]

We are actually using org.grails.plugins:postgresql-extensions:4.6.8 and org.postgresql:postgresql:42.0.0. My PR is against master because we expect to update our library dependencies soon and would likely use org.grails.plugins:postgresql-extensions:6.x. Does that and the fact they have unit tests answer your question or do you need me to do further investigation against something specific? Thanks again.

[ilopmar]

I'm asking because you're modifying how the backslash and the quotes are stored in the db, so I think this is a breaking change and I need to be careful with it. If you plan to upgrade to 6.x to have support for Hibernate 5.2, I can merge this into master and then release 6.1.0.
Keep in mind that 4.6.8 is for Hibernate 4 so until you upgrade you won't be able to have your patch applied. If you need it I can also apply the changes to that branch and release 4.8.0. Let me know what do you prefer.

[butters16]

Forgive my ignorance, but I didn't understand that this parser class was modifying anything in the db. My intent was to work with an in-memory representation and treat the backslashes as part of the next token (when that token is the double-quote or backslash). I thought the scope of this solution was only for the caller. If there is a larger application of this parser that I'm missing please let me know.

To answer your question, it would be great if you could merge this into a 6.1.0 release as well as a 4.8.0 release, but I don't want to be greedy. If I had to pick one, I prefer 6.1.0 so there is at least an eventual migration plan back to this repo. Thanks.

[jglapa]

Hi, I'm the one who raised the issue.
I'm just wondering if using regexp here won't have a negative impact on the performance. I use this extension together with hstore extensively and some of them are pretty large. I fear this could significantly slow it down. Also the patterns are not compiled once but on every set?

[ilopmar]

@butters16 I'm sorry, you're right nothing changes on the database. It's been a while since I used this last time.
@jglapa Do you have the possibility to make some performance test? I think I can release a BUILD-SNAPSHOT release so you can try it out in your application.

[jglapa]

@jglapa Do you have the possibility to make some performance test? I think I can release a BUILD-SNAPSHOT release so you can try it out in your application.

I could benchmark a bit... but right now I'm stuck with version 5.1.0 and Grails 3.2.11. From what I see I could upgrade most likely to 5.2.0 but 6.0 is out of question now due to Hibernate upgrade.

[ilopmar]

The change doesn't seem to complicated so I can ported also to that branch and release 5.3.0 so you can try it. Give me a few days and I'll try to release a version for you.

[jglapa]

The change doesn't seem to complicated so I can ported also to that branch and release 5.3.0 so you can try it. Give me a few days and I'll try to release a version for you.

👌

[butters16]

I'm just wondering if using regexp here won't have a negative impact on the performance.

Good suggestion, String.replace will work just as well, so I'll switch to that and update the PR. You got me thinking about the performance impacts, so I ran my own simple test. Here's a summary of what I found (averaged over 1 million executions, see below):

• 0.452 μs : Original code
• 3.265 μs : String.replaceAll (this PR)
• 2.940 μs : String.replace

So, this change does make the processing 6-7 times slower, but we are talking about the microsecond level, so that seems acceptable to me. I was using a very small example (see below), so maybe @jglapa can help stress test it. The alternative would be to modify the HStoreIterator code, but that seems dangerous without any existing tests.

Also the patterns are not compiled once but on every set?

I tried this as well, but it didn't make an improvement at 2.960 μs. It's possible something else is optimizing here, so I don't plan to make this change.

Here's what I was using to get some metrics (although I had to remove the backslashes for testing the original code since that makes it thrown an exception):

    void "Performance"() {
        expect:
        for (int i = 0; i < 1000000; i++) {
            HstoreParser parser = new HstoreParser('')
            parser.setValue(/"key"=>"\"value\""/)
            parser.asMap()
        }
    }

[butters16]

@jglapa You reported this bug a long time ago. How have you been working around it since then, or are you doing something to avoid the backslashes in the data on the write side?

[jglapa]

@jglapa You reported this bug a long time ago. How have you been working around it since then, or are you doing something to avoid the backslashes in the data on the write side?

Yeah, I wanted to fix this in the HStoreIterator but got ... scared;) We decided to sanitize the input to avoid the issue but would be really great to have that working finally.

[butters16]

@ilopmar Is there anything else I can do to help with merging this PR with official release(s) for 6.1.0 (and possibly 4.8.0)? I updated the PR with one of the suggestions from @jglapa about removing the RegEx.

[ilopmar]

@butters16 Nothing more on your side. I'll try to merge it and release the new versions tonight.

[ilopmar]

I've released the fix in 4.8.0, 5.3.0 and 6.1.0. Thank you very much for your help!

[butters16]

I've released the fix in 4.8.0, 5.3.0 and 6.1.0. Thank you very much for your help!

4.8.0 is working for me. Thank you! I'm glad I could help out!