Add support for point type #283

avinash-anand · 2020-05-23T10:50:27Z

Make sure that:

You have read the contribution guidelines.
You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
You use the code formatters provided here and have them applied to your changes. Don't submit any formatting related changes.
You submit test cases (unit or integration tests) that back your changes.

Issue description

related to issue previously opened support for geometric types - #282

New Public APIs

Additional context

pivotal-issuemaster · 2020-05-23T10:50:29Z

@avinash-anand Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster · 2020-05-23T10:51:49Z

@avinash-anand Thank you for signing the Contributor License Agreement!

Squiry · 2020-05-23T11:21:41Z

If you want driver to know about your codec you should add it to DefaultCodecs.
There are integration test of builtin types in AbstractCodecIntegrationTests, you should add one for point. Probably this will force you to support FORMAT_TEXT.

Are you going to add support for other geometric types here too?

Squiry · 2020-05-23T11:23:47Z

src/main/java/io/r2dbc/postgresql/codec/Point.java

+ * <p>This implements a version of java.awt.Point, except it uses double to represent the coordinates.</p>
+ */
+public class Point {
+    public double x;


Do we really need this to be mutable?

I referred PGpoint of pgjdbc. there it is mentioned as public. I think that perhaps it represents a 2D-point, hence, instead of adding both getters & setters, this value is made mutable.

I can make it private and generate a getter/setter for it.

Those should be rather immutable fields (private final without setters). Thinking of values as atomic and immutable value types is useful as a single point (and future geo types) cannot be changed partially but each update to a point requires a new value object instance.

Squiry · 2020-05-23T11:26:45Z

src/main/java/io/r2dbc/postgresql/codec/PointCodec.java

+            double x = Double.parseDouble(coordinatesAsString[0]);
+            double y = Double.parseDouble(coordinatesAsString[1]);
+            return new Point(x, y);
+        } catch (NumberFormatException | NullPointerException | ArrayIndexOutOfBoundsException e) {


We don't catch NumberFormatException in Numeric codecs and catching NullPointerException and ArrayIndexOutOfBoundsException looks kind of weird.

Actually NumberFormatException was caught in pgjdbc, i thought, the same should be done here as well. Also, these other exceptions were also possible if we receive invalid data, hence caught these exceptions. I can remove it.

What @Squiry said. Let's see whether we have a need for handling such cases at all.

avinash-anand · 2020-05-23T11:48:19Z

If you want driver to know about your codec you should add it to DefaultCodecs.

Sorry, my bad. I'll do it.

There are integration test of builtin types in AbstractCodecIntegrationTests, you should add one for point. Probably this will force you to support FORMAT_TEXT.

Sure. I'll add this test. In pgjdbc, it was marked as binary, hence i forced binary format.

Are you going to add support for other geometric types here too?

Yes, I will add for line and polygon as well. Actually, I started with Point as it was small and simple.

Also, could you please let me know what steps I need to follow to run mvn clean verify locally. On my machine, I got some error. Hence, I ran single test from IDE.

avinash-anand · 2020-05-25T04:19:16Z

@Squiry , @mp911de - There is one issue when i was trying to implement point_array codec and the abstract-array-codec splits incoming data with comma as delimiter. since point is represented as (x,y), it is splitting at wrong place. we may have to look into this as well.

mp911de · 2020-05-25T07:41:19Z

src/main/java/io/r2dbc/postgresql/codec/Point.java

+     * @param x integer amount to add on the x axis
+     * @param y integer amount to add on the y axis
+     */
+    public void translate(int x, int y) {


Those mutation-like methods should return a new instance of Point. Applying mutation to objects always opens up paths for potential bugs.

These methods were present in original driver, so I added it as it is. Do you think these methods should be part of this class now or should I remove them?

Using PGJDBC as a starting point is fine. With the introduction of geo-types to r2dbc-postgressql, we have a chance to improve the actual design. Let's keep these methods in the type but change these to return a new instance.

mp911de · 2020-05-25T07:41:45Z

src/main/java/io/r2dbc/postgresql/codec/Point.java

+     * @param p Point to move to
+     * @see java.awt.Point
+     */
+    public void setLocation(java.awt.Point p) {


We should not use AWT at all.

yes, I was thinking the same. I'll remove this.

mp911de · 2020-05-28T08:38:33Z

There are a few issues, though:

Binary parsing uses 8-byte float representation (see Double.doubleToRawLongBits(…))
Running the tests fails with [08P01] insufficient data left in message))
Some files miss license headers

avinash-anand · 2020-05-30T06:03:50Z

There are a few issues, though:

Binary parsing uses 8-byte float representation (see Double.doubleToRawLongBits(…))

Running the tests fails with [08P01] insufficient data left in message))

I'll try to fix this today.

Some files miss license headers

I'll fix this today.

avinash-anand · 2020-05-30T07:39:34Z

@mp911de - I have fixed the test and updated the files with license. Thanks for pointing out the errors. For some reason, in my local machine - mvn clean verify isn't able to run. I always get below error:-
org.apache.maven.surefire.booter.SurefireBooterForkException: There was an error in the forked process. I can individually run test files in IDE.

Please let me know if anymore changes are needed.

avinash-anand · 2020-06-03T14:04:24Z

@mp911de - Please let me know if this PR is eligible for merging or it needs some changes?

mp911de · 2020-06-03T15:13:06Z

Let me come back by next week to you.

avinash-anand · 2020-06-16T05:54:42Z

Let me come back by next week to you.

@mp911de - Let me know if now is a good time to review this PR?

mp911de · 2020-06-16T07:45:38Z

The code looks good. We will polish up the remaining issues during the merge. Can you please rebase your code, squash all commits into a single commit, and force-push your branch?

avinash-anand · 2020-06-16T09:51:35Z

@mp911de - done.

[#282][#283]

Make Point final and reduce PointCodec visibility. Introduce factory method for Point type. Update documentation. [resolves #283][#282]

mp911de · 2020-06-16T10:34:04Z

Thank you for your contribution. That's merged, polished, and backported now.

Make Point final and reduce PointCodec visibility. Introduce factory method for Point type. Update documentation. [resolves pgjdbc#283][pgjdbc#282]

Squiry reviewed May 23, 2020

View reviewed changes

mp911de reviewed May 25, 2020

View reviewed changes

mp911de self-assigned this May 28, 2020

mp911de modified the milestone: 0.8.3.RELEASE May 28, 2020

mp911de added the type: enhancement A general enhancement label Jun 16, 2020

mp911de added this to the 0.8.4.RELEASE milestone Jun 16, 2020

mp911de changed the title ~~feature: adding support for point type~~ Add support for point type Jun 16, 2020

avinash-anand force-pushed the issue-282-point branch from 671bade to 8bd640a Compare June 16, 2020 09:41

feature: adding support for point type

69d1258

avinash-anand force-pushed the issue-282-point branch from 8bd640a to 69d1258 Compare June 16, 2020 09:50

mp911de pushed a commit that referenced this pull request Jun 16, 2020

feature: adding support for point type

ddb9f2f

[#282][#283]

mp911de added a commit that referenced this pull request Jun 16, 2020

Polishing

3d27a0a

Make Point final and reduce PointCodec visibility. Introduce factory method for Point type. Update documentation. [resolves #283][#282]

mp911de merged commit 69d1258 into pgjdbc:master Jun 16, 2020

mp911de mentioned this pull request Jun 16, 2020

Support for geospatial types - point, line, box, polygon etc #282

Closed

7 tasks

avinash-anand deleted the issue-282-point branch June 16, 2020 10:46

avinash-anand pushed a commit to avinash-anand/r2dbc-postgresql that referenced this pull request Jul 25, 2020

Polishing

6f5315b

Make Point final and reduce PointCodec visibility. Introduce factory method for Point type. Update documentation. [resolves pgjdbc#283][pgjdbc#282]

avinash-anand pushed a commit to avinash-anand/r2dbc-postgresql that referenced this pull request Jul 25, 2020

Polishing

cdf46bd

Make Point final and reduce PointCodec visibility. Introduce factory method for Point type. Update documentation. [resolves pgjdbc#283][pgjdbc#282]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for point type #283

Add support for point type #283

avinash-anand commented May 23, 2020 •

edited

Loading

pivotal-issuemaster commented May 23, 2020

pivotal-issuemaster commented May 23, 2020

Squiry commented May 23, 2020 •

edited

Loading

Squiry May 23, 2020

avinash-anand May 23, 2020

avinash-anand May 23, 2020

mp911de May 25, 2020

Squiry May 23, 2020

avinash-anand May 23, 2020

mp911de May 25, 2020

avinash-anand commented May 23, 2020

avinash-anand commented May 25, 2020

mp911de May 25, 2020

avinash-anand May 25, 2020

mp911de May 25, 2020

mp911de May 25, 2020

avinash-anand May 25, 2020

mp911de commented May 28, 2020

avinash-anand commented May 30, 2020

avinash-anand commented May 30, 2020

avinash-anand commented Jun 3, 2020

mp911de commented Jun 3, 2020

avinash-anand commented Jun 16, 2020

mp911de commented Jun 16, 2020

avinash-anand commented Jun 16, 2020

mp911de commented Jun 16, 2020

Add support for point type #283

Add support for point type #283

Conversation

avinash-anand commented May 23, 2020 • edited Loading

Issue description

New Public APIs

Additional context

pivotal-issuemaster commented May 23, 2020

pivotal-issuemaster commented May 23, 2020

Squiry commented May 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avinash-anand commented May 23, 2020

avinash-anand commented May 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mp911de commented May 28, 2020

avinash-anand commented May 30, 2020

avinash-anand commented May 30, 2020

avinash-anand commented Jun 3, 2020

mp911de commented Jun 3, 2020

avinash-anand commented Jun 16, 2020

mp911de commented Jun 16, 2020

avinash-anand commented Jun 16, 2020

mp911de commented Jun 16, 2020

avinash-anand commented May 23, 2020 •

edited

Loading

Squiry commented May 23, 2020 •

edited

Loading