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

feat: primitive arrays #887

Merged
merged 12 commits into from Dec 19, 2017

Conversation

Projects
None yet
6 participants
@bokken
Copy link
Member

bokken commented Jul 27, 2017

Add support for primitive arrays.
This is a replacement for PR 872.

BO8979 and others added some commits Jul 22, 2017

feat: minimal PR to support primitive arrays
This is a minimal PR for purposes of discussion to support primitive
arrays as part of issue #871

feat: minimal PR to support primitive arrays

Improve StringBuilder init sizes.

feat: support primitive arrays

Add unit tests.
Provide binary representation.
Add javadoc.

feat: support primitive arrays

Use Oid rather than type name.
More unit tests.

feat: support primitive arrays

Address checkstyle issues.

feat: support primitive arrays

Address checkstyle issues.
Fix an issue with creating array string.
Add support for boolean[].
Optimize mapping from String values to boolean.

feat: support primitive arrays

Address checkstyle issues.

feat: support primitive arrays

Use java 6 syntax.

feat: support primitive arrays

checkstyle in existing test

feat: support primitive arrays

checkstyle in existing test

test: migrate tests to JUnit 4

fixes #738
fixes #205

test: assume minimum server version 8.3 testing autosave with ALTER

Prior to 8.3, PostgreSQL didn't invalidate cached plans following DDL
changes. This is mentioned in the 8.3 release notes as

    Automatically re-plan cached queries when table definitions change
    or statistics are updated

https://www.postgresql.org/docs/8.3/static/release-8-3.html

For details, see PostgreSQL commit b9527e9840 and friends.

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=b9527e984092e838790b543b014c0c2720ea4f11

test: assume minimum server version 8.3 when testing with uuid

The UUID datatype was added to PostgreSQL in version 8.3.

https://www.postgresql.org/docs/8.3/static/release-8-3.html

refactor: remove unused import

test: fix StringTypeParameterTest to skip preferQueryMode=simple

Simple query mode does not send data type over the wire, so errors like
"column \"m\" is of type mood but expression is of type character varying" never happen

chore: install PostgreSQL 9.1 to Trusty builds via apt, and use Precise for Java 6

Travis deprecates Precise. See
https://blog.travis-ci.com/2017-07-11-trusty-as-default-linux-is-coming

sudo: required for dist: precise will still be supported for a while, however no updates to that image is planned

Java 6 is not included into Trusty image, and installers no longer work
(see http://www.webupd8.org/2017/06/why-oracle-java-7-and-6-installers-no.html) since
Oracle removed the installer files from public access.

refactor: remove useless checks in the tests

remove useless checks for protocol version in the tests.

style: update checkstyle + turn on some rules (#847)

1. Update checkstyle to 7.8.2
2. Add checkstyle module "RedundantModifier"
3. Uncomment checkstyle modules "ArrayTypeStyle", "ModifierOrder"

test: make StringTypeParameterTest 8.3+ since 8.2 misses enum types (#882)

PostgreSQL 8.2 tests at Travis should be green now, so removing allow_failures configuration

test: assume integer datetimes for timestamp tests (#873)

Float timestamp equality comparisons like comparisons with any float
are problematic if they don't take into account their imprecise
nature. Some timestamp tests produce false negative failures for
servers compiled with float timestamps. Use JUnit assumptions to skip
these tests if the server doesn't have integer datetimes.

This addresses #12 in part in
that it omits (most of) the failing tests, though it leaves these
timestamp behaviors untested under float timestamps, which was the
default prior to PostgreSQL 8.4.

fix: named statements were used when fetchSize was non-zero and prepareThreshold=0 (#870)

Non-zero fetchSize triggers use of named portals (for subsequent fetch requests),
however named portals does not require to use named statements.

As per PostgreSQL documentation, named portals are automatically closed as transaction completes, so named portals should play well with transaction-based poolers.

fixes #869

test: skip ConcurrentStatementFetch for PostgreSQL < 8.4 (#884)

PostgreSQL 8.2 is known to close named portals unexpectedly, so we just ignore the test.
The following scenario might be affected: old backend version + setFetchSize + interleaved ResultSet processing

This is a follow-up for #870 and #869 for PostgreSQL < 8.4

honour PGPORT, PGHOST, PGDBNAME in connection properties (#862)

* honour PGPORT, PGHOST, PGDBNAME in connection properties

feat: support primitive arrays

use string array representation when simple query mode selected

feat: support primitive arrays

Checkstyle

feat: support primitive arrays

More unit test coverage

feat: support primitive arrays

Checkstyle

test: migrate tests to Junit4 (#883)

Migrate XATestSuite, Jdbc2TestSuite, Jdbc4TestSuite, ExtensionsTestSuite test suites

see #205

feat: support primitive arrays

Performance optimizations.
Some renaming.

feat: support primitive arrays

Performance optimizations in TypeInfoCache.
Checkstyle issue.

feat: support primitive arrays

Initialize maps in TypeInfoCache to appropriate size.

feat: support primitive arrays

Checkstyle

feat: primitive arrays

revert changes not directly related to supporting
primitive arrays
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 27, 2017

Codecov Report

Merging #887 into master will increase coverage by 0.48%.
The diff coverage is 90.94%.

@@             Coverage Diff              @@
##             master     #887      +/-   ##
============================================
+ Coverage     66.35%   66.83%   +0.48%     
- Complexity     3607     3637      +30     
============================================
  Files           168      169       +1     
  Lines         15355    15593     +238     
  Branches       2483     2529      +46     
============================================
+ Hits          10189    10422     +233     
+ Misses         3985     3976       -9     
- Partials       1181     1195      +14
private static final Map<Class, PrimitiveArraySupport> ARRAY_CLASS_TOSTRING = new HashMap<Class, PrimitiveArraySupport>(9);

static {
ARRAY_CLASS_TOSTRING.put(long[].class, LONG_ARRAY_TOSTRING);

This comment has been minimized.

@panchenko

panchenko Jul 28, 2017

As binary serialization is also supported, I think the TOSTRING part can be removed from all the identifiers.

This comment has been minimized.

@bokken
Object array = readBinaryArray(1, 0);

if (PrimitiveArraySupport.isSupportedPrimitiveArray(array)) {
fieldString = PrimitiveArraySupport.arrayToString(connection.getTypeInfo().getArrayDelimiter(oid), array);

This comment has been minimized.

@panchenko

panchenko Jul 28, 2017

might be like this:

arraySupport = PrimitiveArraySupport.getArraySupport(array)
if (arraySupport != null) {
   fieldString = arraySupport.toArrayString(array)
}

The static PrimitiveArraySupport.arrayToString() method can be removed then.

This comment has been minimized.

@bokken

bokken Jul 28, 2017

Member

Done.
The static utility arrayToString method was used more before binary support was added. I did not realize it had dropped to only 1 use when binary representation became preferred.

@@ -295,27 +313,34 @@ public void testUUIDArray() throws SQLException {
@Test
public void testSetObjectFromJavaArray() throws SQLException {
String[] strArray = new String[]{"a", "b", "c"};
Object[] objCopy = new Object[strArray.length];
System.arraycopy(strArray, 0, objCopy, 0, strArray.length);

This comment has been minimized.

@panchenko

panchenko Jul 28, 2017

can be replaced with Arrays.copyOf(strArray, strArray.length, Object[].class)

This comment has been minimized.

@bokken

bokken Jul 28, 2017

Member

Done.
Out of curiosity, why do you prefer the Arrays wrapper? These are functionally equivalent things, skipping some of the reflection stuff that Arrays has to do (or in this specific case because it is Object[], just a class identity comparison).

This comment has been minimized.

@panchenko

panchenko Jul 28, 2017

It's shorter, so more readable.

/**
* Defines public PostgreSQL extensions to create {@link Array} objects.
*/
public interface PGArraySupport {

This comment has been minimized.

@panchenko

panchenko Jul 28, 2017

How much this level of indirection is useful? How many methods can be here potentially? I would suggest declaring the method directly in PGConnection.

This comment has been minimized.

@bokken

bokken Jul 28, 2017

Member

@vlsi seemed to want at least 1 more method.
It was not clear to me if it was just one more or several more.
I do not know at what number you would want to group together with a specific interface. I would think at 3 or so, but I do not really care either way.

This comment has been minimized.

@panchenko

panchenko Jul 28, 2017

This is a little bit closer to Connection if comparing with other extensions, as there is java.sql.Connection.createArrayOf() already.

feat: primitive arrays
code review comments
@bokken

This comment has been minimized.

Copy link
Member

bokken commented Jul 28, 2017

It still appears that the code coverage report is not including the new unit test class i added. How is it identifying what tests to run?

@AlexElin

This comment has been minimized.

Copy link
Contributor

AlexElin commented Jul 28, 2017

@bokken add your test class to the corresponding test suite

bokken added some commits Jul 28, 2017

feat: primitive arrays
code review comments
feat: primitive arrays
documentation
@bokken

This comment has been minimized.

Copy link
Member

bokken commented Aug 16, 2017

@davecramer, @vlsi, @panchenko, is there anything else needed, or can this get merged in?

Thanks.

@bokken

This comment has been minimized.

Copy link
Member

bokken commented Aug 21, 2017

@vlsi and @davecramer any update on when this could get merged in?
I would like to be able to start using this without having to manage a fork.

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Aug 22, 2017

@bokken LGTM. @vlsi is on vacation. He should be back in a week

@bokken

This comment has been minimized.

Copy link
Member

bokken commented Aug 23, 2017

Thanks @davecramer

@bokken

This comment has been minimized.

Copy link
Member

bokken commented Oct 14, 2017

@vlsi, any update/eta on when this could get merged in?

@bokken

This comment has been minimized.

Copy link
Member

bokken commented Nov 20, 2017

@davecramer, @vlsi, any update/eta on when this could get merged in?

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Nov 21, 2017

@bokken Sorry, I'll commit to reviewing this on Wed. Thanks for your patience

`int[]` | `int4[]`
`long[]` | `int8[]`
`float[]` | `float4[]`
`double[]` | `float[]8`

This comment has been minimized.

@vlsi

vlsi Nov 22, 2017

Member

float[]8 typo

This comment has been minimized.

@bokken

bokken Nov 22, 2017

Member

fixed

@bokken

This comment has been minimized.

Copy link
Member

bokken commented Dec 12, 2017

@davecramer, @vlsi, any update/eta on when this could get merged in?

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Dec 13, 2017

@bokken there are some subtle semantic changes here. Specifically setObject(1, String[], Types.Array) used to fail, now it doesn't was this intentional?

@bokken

This comment has been minimized.

Copy link
Member

bokken commented Dec 13, 2017

@davecramer, yes that was intentional and is called out in the documentation:

https://github.com/pgjdbc/pgjdbc/pull/887/files?diff=unified#diff-b8b4949d263ba9fd44d157ae61de7584R19

The listed array types can go through setObject

* @throws SQLException If for some reason the array cannot be created.
* @see java.sql.Connection#createArrayOf(String, Object[])
*/
Array createArrayOf(String typeName, Object elements) throws SQLException;

This comment has been minimized.

@vlsi

vlsi Dec 14, 2017

Member

Can Object elements be null?

This comment has been minimized.

@bokken

bokken Dec 14, 2017

Member

No it cannot.
Similar to how createArrayOf(String typeName, Object[] elements) cannot take null elements either.

This comment has been minimized.

@vlsi

vlsi Dec 15, 2017

Member

@bokken , here's Java 8 documentation:

https://docs.oracle.com/javase/8/docs/api/java/sql/Connection.html#createArrayOf-java.lang.String-java.lang.Object:A-

SQLException - if a database error occurs, the JDBC type is not appropriate for the typeName and the conversion is not supported, the typeName is null or this method is called on a closed connection

It does forbid null for typeName, however I do not see how it forbids null for elements

The added PGConnection.createArrayOf(String, Object) is a public API, and I think nullability documentation is a must.

PS. Sorry for the long-standing discussion, however we'd better be picky when adding new APIs.

This comment has been minimized.

@bokken

bokken Dec 15, 2017

Member

I was speaking to the PGConnection implementation of createArrayOf, which also throws a NullPointerException if given null elements.
As this was not documented there, I did not document here.

https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java#L1149

This comment has been minimized.

@bokken

bokken Dec 15, 2017

Member

Perhaps both methods should return null if elements is null?
I was simply expecting/providing symmetry with the existing implementation.

This comment has been minimized.

@vlsi

vlsi Dec 15, 2017

Member

NullPointerException if given null elements might be an existing bug.

This comment has been minimized.

@bokken

bokken Dec 15, 2017

Member

It appears that it is possible to create a PGArray representing a null array. Perhaps that is the solution?

This comment has been minimized.

@davecramer

davecramer Dec 16, 2017

Member

So it is possible to create a null array in postgres so seems reasonable to allow it

select null::int[];
 int4
------
@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Dec 15, 2017

@bokken can you fix the conflict and I'll commit this. Thanks

Merge remote-tracking branch 'upstream/master' into primitives
# Conflicts:
#	pgjdbc/src/test/java/org/postgresql/test/jdbc2/Jdbc2TestSuite.java
@bokken

This comment has been minimized.

Copy link
Member

bokken commented Dec 15, 2017

@davecramer, @vlsi,
It might be helpful to have an @since javadoc tag on the new createArrayOf method on PGConnection interface. What release would this be targeted for? 42.2?

I have fixed the conflict.

Let me know if you would like me to add the @since doc and/or make any changes to doc or implementation around handling of null elements.

bokken added some commits Dec 15, 2017


/**
* Creates an {@link Array} wrapping <i>elements</i>. This is similar to
* java.sql.Connection#createArrayOf(String, Object[]), but also provides

This comment has been minimized.

@vlsi

vlsi Dec 15, 2017

Member

{@link ...createArrayOf...}

This comment has been minimized.

@bokken
* java.sql.Connection#createArrayOf(String, Object[]), but also provides
* support for primitive arrays.
*
* @param typeName

This comment has been minimized.

@vlsi

vlsi Dec 15, 2017

Member

Please add nullability description

This comment has been minimized.

@bokken
*
* @param typeName
* The SQL name of the type to map the <i>elements</i> to.
* @param elements

This comment has been minimized.

@vlsi

vlsi Dec 15, 2017

Member

Please add nullability description

This comment has been minimized.

@bokken

bokken added some commits Dec 15, 2017

@bokken

This comment has been minimized.

Copy link
Member

bokken commented Dec 16, 2017

@bokken

This comment has been minimized.

Copy link
Member

bokken commented Dec 19, 2017

@davecramer the conflict has been resolved.
@vlsi I believe I have covered all the doc updates you requested.
Null elements are now handled (with PGArray object wrapping null) for both the new method and the existing jdbc standard method.

@vlsi vlsi merged commit 3e0491a into pgjdbc:master Dec 19, 2017

2 checks passed

codecov/project 66.83% (+0.48%) compared to 405f14e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vlsi vlsi removed the needs-review label Dec 19, 2017

@vlsi vlsi added this to the 42.2.0 milestone Dec 19, 2017

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

feat: primitive arrays (pgjdbc#887)
Support mapping of Java types short[], int[], long[], float[], double[], boolean[], String[] to relevant types in PostgreSQL via org.postgresql.PGConnection.createArrayOf(String, Object) API.

closes pgjdbc#871

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

feat: primitive arrays (pgjdbc#887)
Support mapping of Java types short[], int[], long[], float[], double[], boolean[], String[] to relevant types in PostgreSQL via org.postgresql.PGConnection.createArrayOf(String, Object) API.

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