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

Add core type delimiters into cache for array type OIDs. #1416

Merged
merged 2 commits into from Feb 22, 2019

Conversation

Projects
None yet
5 participants
@doxavore
Copy link
Contributor

commented Feb 20, 2019

In addition to caching the underlying core type mapped to a comma,
cache their array equivalent types to avoid querying from the server.

Fixes #1415.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Does mvn checkstyle:check pass ?

Changes to Existing Features:

  • Does this break existing behaviour? If so please explain.
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?
Add core type delimiters into cache for array type OIDs.
In addition to caching the underlying core type mapped to a comma,
cache their array equivalent types to avoid querying from the server.

Fixes #1415.
@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 20, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Feb 20, 2019

Codecov Report

Merging #1416 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1416      +/-   ##
============================================
- Coverage     68.72%   68.71%   -0.02%     
+ Complexity     3895     3894       -1     
============================================
  Files           179      179              
  Lines         16400    16401       +1     
  Branches       2669     2669              
============================================
- Hits          11271    11270       -1     
  Misses         3882     3882              
- Partials       1247     1249       +2
@davecramer

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Seems simple enough...

LGTM

@@ -152,6 +152,7 @@ public synchronized void addCoreType(String pgTypeName, Integer oid, Integer sql
//
Character delim = ',';
arrayOidToDelimiter.put(oid, delim);
arrayOidToDelimiter.put(arrayOid, delim);

This comment has been minimized.

Copy link
@bokken

bokken Feb 20, 2019

Member

The arrayOidToDelimiter map should be initialized to 2x size to allow for these additional entries

This comment has been minimized.

Copy link
@davecramer

davecramer Feb 20, 2019

Member

good catch...

This comment has been minimized.

Copy link
@doxavore

doxavore Feb 20, 2019

Author Contributor

You mean in the constructor, or somewhere else? It doesn't seem to have a size there to begin with (not that it wouldn't be good to add one there I suppose).

This comment has been minimized.

Copy link
@doxavore

doxavore Feb 20, 2019

Author Contributor

If you'd like to see arrayOidToDelimeter = new HashMap(types.length * 2) in the constructor, I can do that. But would it make sense to do the same with pgNameToOid, pgNameToJavaClass, pgNameToPgObject, and types.length * 1 for pgArrayToPgType?

Also, should we consider updating getArrayDelimiter to fall back to arrayOidToDelimiter.get(pgArrayToPgType.get(oid)) before hitting the database?

Again, I'm not sure what the intended behavior is for getting delimiters for the array-type OID instead of the underlying type in the first place; I'd expected getArrayDelimiter to be receiving the OID for int8 and not its array companion when the field is an int8[], and the OID for int8[] in multidimensional arrays. But even if that's correct, it's a larger change than is probably worth doing here.

This comment has been minimized.

Copy link
@bokken

bokken Feb 20, 2019

Member

If you'd like to see arrayOidToDelimeter = new HashMap(types.length * 2) in the constructor, I can do that. But would it make sense to do the same with pgNameToOid, pgNameToJavaClass, pgNameToPgObject, and types.length * 1 for pgArrayToPgType?

I would vote arrayOidToDelimeter to be types.length * 2.5 and the rest to be types.length * 1.5. The PgConnection class also adds some values every time.

Also, should we consider updating getArrayDelimiter to fall back to arrayOidToDelimiter.get(pgArrayToPgType.get(oid)) before hitting the database?

I am somewhat ambivalent here. I think the currently proposed change to populate the map for these core types would appear to cover the vast majority of cases. Adding this would avoid a db query per connection lifetime for non-core types?

I'd expected getArrayDelimiter to be receiving the OID for int8 and not its array companion when the field is an int8[], and the OID for int8[] in multidimensional arrays.

As I recall, this is driven by what direction we are going (inserting vs. retrieving). On inserts we are primarily starting with the base type int8 whether single dimension or multi-dimensional. On retrieves I seem to recall that the column type is described as int8[], again regardless of single or multi-dimensional.

This comment has been minimized.

Copy link
@doxavore

doxavore Feb 21, 2019

Author Contributor

Thanks for the feedback and explanation. I've updated per your comments in c0921a0. I agree that there's probably no need to fall back to arrayOidToDelimiter.get(pgArrayToPgType.get(oid)) if we're doing everything else right.

Working through addCoreType it looks like the other arrays are added to twice - once for pgTypeName and again for pgArrayTypeName (eg. int8 and _int8, respectively). That's why I suggested types.length * 2 for the other arrays.

@AppVeyorBot

This comment has been minimized.

Copy link

commented Feb 21, 2019

@doxavore

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Anything else I should be doing to help shepherd this through? I'd hate to miss the next release. :>

@davecramer

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

no, that's fine I hadn't see the second commit.

@davecramer davecramer merged commit 6a0960a into pgjdbc:master Feb 22, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.