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

perf: improve parsing performance of JDBC-style { call ...} calls #1233

Merged
merged 1 commit into from Jul 1, 2018

Conversation

Projects
None yet
5 participants
@benbenw
Copy link
Contributor

benbenw commented Jun 30, 2018

add some tests on Parser#modifyJdbcCall
small optimization on Parser#modifyJdbcCall
remove support for version < 8.1 in Parser#modifyJdbcCall

@benbenw benbenw changed the title perf: Parser#modifyJdbcCall improvements WIP: Parser#modifyJdbcCall improvements Jun 30, 2018

if (serverVersion < 80100 /* 8.1 */ || protocolVersion != 3) {
sql = "select " + jdbcSql.substring(startIndex, endIndex) + " as result";
sql = new StringBuilder().append("select ").append(jdbcSql, startIndex, endIndex).append(suffix).toString();

This comment has been minimized.

@bokken

bokken Jun 30, 2018

Member

Is it worthwhile to make minor optimizations (with small negative impact on readability) for versions prior to 8.1?

This comment has been minimized.

@benbenw

benbenw Jun 30, 2018

Author Contributor

Up to you !
is this driver officially supports version prior to 8.2 ?

This comment has been minimized.

@vlsi

vlsi Jun 30, 2018

Member

Technically speaking, only 8.2+ are supported.
So the branch could be just removed

This comment has been minimized.

@davecramer

davecramer Jun 30, 2018

Member

+1 to removing it

This comment has been minimized.

@benbenw

benbenw Jun 30, 2018

Author Contributor

could you have 8.2+ with protocol version != 3 ?

This comment has been minimized.

@vlsi

vlsi Jun 30, 2018

Member

Only protocol=3 is supported for now.
There was protocol=2 (for <8.2), however the implementation has already been removed.

This comment has been minimized.

@benbenw

benbenw Jun 30, 2018

Author Contributor

if not I'll remove the branch and the associated test

@benbenw benbenw force-pushed the benbenw:modifyJdbcCall branch from fc300d5 to 34334b9 Jun 30, 2018

@vlsi vlsi added this to the 42.2.3 milestone Jun 30, 2018

@benbenw benbenw force-pushed the benbenw:modifyJdbcCall branch from 34334b9 to ed4c305 Jun 30, 2018

@bokken

bokken approved these changes Jun 30, 2018

@benbenw benbenw force-pushed the benbenw:modifyJdbcCall branch from ed4c305 to b25abc7 Jun 30, 2018

@@ -1024,18 +1026,18 @@ public static JdbcCallParseInfo modifyJdbcCall(String jdbcSql, boolean stdString

int closing = s.indexOf(')', opening);
for (int j = opening; j < closing; j++) {
if (!Character.isWhitespace(sb.charAt(j))) {
if (!Character.isWhitespace(sb.charAt(j + prefixLength))) {

This comment has been minimized.

@vlsi

vlsi Jun 30, 2018

Member

@benbenw , please remove computation s.indexOf(')', opening) as well
It can be incorporated into the loop, so the loop could stop either at the first non-whitespace character or at ). On top of that, it makes sense to add j < sb.length() condition to avoid AIOBE

@Test
public void testModifyJdbcCall() throws SQLException {
assertEquals("select * from pack_getValue(?,?) as result", Parser.modifyJdbcCall("{ ? = call pack_getValue(?) }", true, ServerVersion.v9_6.getVersionNum(), 3).getSql());
assertEquals("select * from pack_getValue(?) as result", Parser.modifyJdbcCall("{ ? = call pack_getValue()}", true, ServerVersion.v9_6.getVersionNum(), 3).getSql());

This comment has been minimized.

@vlsi

vlsi Jun 30, 2018

Member

Please add a case like { ? = call pack_getValue}

This comment has been minimized.

@benbenw

benbenw Jun 30, 2018

Author Contributor

done

@benbenw benbenw force-pushed the benbenw:modifyJdbcCall branch from b25abc7 to 9ab110a Jun 30, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 30, 2018

Codecov Report

Merging #1233 into master will decrease coverage by 0.02%.
The diff coverage is 90%.

@@             Coverage Diff              @@
##             master    #1233      +/-   ##
============================================
- Coverage     68.68%   68.66%   -0.03%     
+ Complexity     3834     3832       -2     
============================================
  Files           173      173              
  Lines         16000    16002       +2     
  Branches       2609     2608       -1     
============================================
- Hits          10989    10987       -2     
- Misses         3780     3783       +3     
- Partials       1231     1232       +1

@benbenw benbenw force-pushed the benbenw:modifyJdbcCall branch from 9ab110a to bf44692 Jun 30, 2018

@benbenw benbenw changed the title WIP: Parser#modifyJdbcCall improvements perf: Parser#modifyJdbcCall improvements Jun 30, 2018

add some tests on Parser#modifyJdbcCall
small optimization on Parser#modifyJdbcCall
remove support for version < 8.1 in Parser#modifyJdbcCall

@benbenw benbenw force-pushed the benbenw:modifyJdbcCall branch from bf44692 to 7978cc7 Jul 1, 2018

@benbenw

This comment has been minimized.

Copy link
Contributor Author

benbenw commented Jul 1, 2018

rebased

@vlsi vlsi changed the title perf: Parser#modifyJdbcCall improvements perf: improve parsing performance of JDBC-style { call ...} calls Jul 1, 2018

@vlsi

vlsi approved these changes Jul 1, 2018

@vlsi vlsi merged commit 435e2f7 into pgjdbc:master Jul 1, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018

perf: improve parsing performance of JDBC-style { call ...} calls (pg…
…jdbc#1233)

Avoid intermediate string allocations in Parser#modifyJdbcCall
Remove support for version < 8.1 in Parser#modifyJdbcCall

rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018

perf: improve parsing performance of JDBC-style { call ...} calls (pg…
…jdbc#1233)

Avoid intermediate string allocations in Parser#modifyJdbcCall
Remove support for version < 8.1 in Parser#modifyJdbcCall
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.