[ISSUE #22] Created spoint_deg, scircle_deg, spoly_deg functions. #38

dura0ok · 2023-07-31T13:20:14Z

vitcpp · 2023-07-31T15:33:41Z

@stepan-neretin7 In general, I'm ok with your patch except of some cosmetic issues (float -> float8 is important). To create a complete patch I would ask to change the version to 1.3.0 (API is changed) and create a new upgrade script with name pg_sphere--1.2.3--1.3.0.sql.in in upgrade_scripts subdirectory. Makefile should also be updated to add support for the new upgrade script. Upgrade scripts are used to alter the existing schema of previous version to the new schema. In our case we have to create new functions in the new upgrade script. Thank you!

expected/circle.out

src/pgs_util.h

dura0ok · 2023-08-01T06:14:24Z

@vitcpp I have updated the version

pgs_types.sql.in

dura0ok · 2023-08-01T07:32:26Z

I also added the spoly_deg function, which accepts, as in the pr of 5 years ago, an array of float8, where there are consecutive pairs in the sequence of two float8 per point for the polygon

esabol · 2023-08-02T00:38:21Z

I also added the spoly_deg function, which accepts, as in the pr of 5 years ago, an array of float8, where there are consecutive pairs in the sequence of two float8 per point for the polygon

Thank you! I was just going to suggest that adding a spoly_deg() for the sake of completeness might be a good idea. 😄

dura0ok · 2023-08-02T02:12:20Z

I also added the spoly_deg function, which accepts, as in the pr of 5 years ago, an array of float8, where there are consecutive pairs in the sequence of two float8 per point for the polygon

Thank you! I was just going to suggest that adding a spoly_deg() for the sake of completeness might be a good idea. smile

Sorry, I didn't really get your message. So I guessed your thought? :D
And how do you like pr in general, how about approve?

esabol · 2023-08-02T02:22:04Z

And how do you like pr in general, how about approve?

Sorry, I thought I already did. Approved!

esabol · 2023-08-02T02:30:05Z

Oh, sorry, I found one more minor thing: I saw you fixed the "radius must be not greater than 90 degrees" error message, but there is another version of that error on line 84 of src/circle.c. Could you fix line 84 similar to how you fixed line 366?

dura0ok · 2023-08-02T03:08:39Z

Oh, sorry, I found one more minor thing: I saw you fixed the "radius must be not greater than 90 degrees" error message, but there is another version of that error on line 84 of src/circle.c. Could you fix line 84 similar to how you fixed line 366?

Of course, I've already fixed it. However, it is strange that there are no tests specifically for this error in circle.sql.
I can add if necessary and it will be appropriate in this pull request

vitcpp · 2023-08-02T06:05:45Z

@stepan-neretin7 I added some review comments. Most of the comments are minor and related to the code formatting. I would propose to work out these comments. Another performance issue I've found is in spherepoly_deg(). It will work as it is now but it is not an effective solution. Anyway, I'm ok to apply PR without redesign of this function.

src/point.c

src/pgs_util.h

upgrade_scripts/pg_sphere--1.2.3--1.3.0.sql.in

src/polygon.c

vitcpp · 2023-08-02T06:00:47Z

src/polygon.c

+    for (i = 0; i < num_elements - 1; i += 2)
+    {
+        point = DirectFunctionCall2(
+            spherepoint_from_long_lat_deg, Float8GetDatum(array_data[i]), Float8GetDatum(array_data[i+1])


It is not an effective solution. spherepoint_from_long_lat_deg returns palloc-ed spoint. Later this spoint is copied into already pallocated points array. I propose to pfree it after line 892. It will work but I would rather think how to avoid unecessary palloc.

src/polygon.c

pgs_types.sql.in

dura0ok · 2023-08-02T18:11:02Z

I corrected all the comments
Look again, please @esabol

pgs_point.sql.in

pgs_types.sql.in

src/circle.c

src/pgs_util.h

src/polygon.c

upgrade_scripts/pg_sphere--1.2.3--1.3.0.sql.in

src/point.h

src/polygon.c

dura0ok · 2023-08-03T05:08:06Z

Sorry for trivial coding style issues. I fixed it @esabol

pgs_types.sql.in

src/polygon.c

vitcpp · 2023-08-03T19:43:44Z

src/polygon.c

+		);
+	}
+	PG_RETURN_POINTER(spherepoly_from_array(points, np));
+


Remove unecessary blank line please.

src/polygon.c

t.patch

esabol

Looks good to me!

dura0ok force-pushed the deg_functions branch from cdec17d to edf18f5 Compare July 31, 2023 13:24

esabol reviewed Jul 31, 2023

View reviewed changes

expected/circle.out Outdated Show resolved Hide resolved

esabol reviewed Jul 31, 2023

View reviewed changes

src/pgs_util.h Outdated Show resolved Hide resolved

dura0ok force-pushed the deg_functions branch 3 times, most recently from aa46131 to e0c76c4 Compare August 1, 2023 05:30

dura0ok requested a review from esabol August 1, 2023 05:30

dura0ok force-pushed the deg_functions branch 2 times, most recently from 048e068 to 5ccfedb Compare August 1, 2023 05:44

esabol reviewed Aug 1, 2023

View reviewed changes

pgs_types.sql.in Outdated Show resolved Hide resolved

dura0ok force-pushed the deg_functions branch from 5ccfedb to c00123d Compare August 1, 2023 07:22

dura0ok requested a review from esabol August 1, 2023 07:26

esabol approved these changes Aug 2, 2023

View reviewed changes

esabol mentioned this pull request Aug 2, 2023

[ISSUE #16] Implemented a function and tests to extract vertices from spoly by index #37

Merged

dura0ok force-pushed the deg_functions branch from c00123d to 367a665 Compare August 2, 2023 03:06

vitcpp reviewed Aug 2, 2023

View reviewed changes

dura0ok force-pushed the deg_functions branch 3 times, most recently from 30a17c7 to df45187 Compare August 2, 2023 09:53

dura0ok requested review from esabol and vitcpp August 2, 2023 10:40

esabol suggested changes Aug 2, 2023

View reviewed changes

src/polygon.c Outdated Show resolved Hide resolved

src/polygon.c Show resolved Hide resolved

src/polygon.c Outdated Show resolved Hide resolved

pgs_types.sql.in Outdated Show resolved Hide resolved

dura0ok force-pushed the deg_functions branch from df45187 to f2db6f8 Compare August 2, 2023 18:09

dura0ok requested a review from esabol August 2, 2023 18:11

esabol suggested changes Aug 3, 2023

View reviewed changes

dura0ok force-pushed the deg_functions branch from f2db6f8 to 2a7cdc0 Compare August 3, 2023 05:07

dura0ok requested a review from esabol August 3, 2023 05:08

vitcpp requested changes Aug 3, 2023

View reviewed changes

pgs_types.sql.in Outdated Show resolved Hide resolved

src/polygon.c Show resolved Hide resolved

src/polygon.c Show resolved Hide resolved

src/polygon.c Outdated Show resolved Hide resolved

src/polygon.c Outdated Show resolved Hide resolved

dura0ok force-pushed the deg_functions branch from 2a7cdc0 to e5ffb50 Compare August 3, 2023 10:17

dura0ok requested a review from vitcpp August 3, 2023 10:17

vitcpp requested changes Aug 3, 2023

View reviewed changes

src/polygon.c Outdated

);

}

PG_RETURN_POINTER(spherepoly_from_array(points, np));

Copy link

Contributor

vitcpp Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unecessary blank line please.

vitcpp reviewed Aug 3, 2023

View reviewed changes

src/polygon.c Outdated Show resolved Hide resolved

dura0ok force-pushed the deg_functions branch 2 times, most recently from 9d8b007 to 4199d35 Compare August 4, 2023 04:49

dura0ok requested a review from vitcpp August 4, 2023 05:08

dura0ok force-pushed the deg_functions branch from 4199d35 to 20aebb1 Compare August 4, 2023 06:47

vitcpp requested changes Aug 4, 2023

View reviewed changes

t.patch Outdated Show resolved Hide resolved

esabol approved these changes Aug 4, 2023

View reviewed changes

[ISSUE postgrespro#22] Created spoint_deg, scircle_deg functions.

118fe52

dura0ok force-pushed the deg_functions branch from 20aebb1 to 118fe52 Compare August 4, 2023 08:16

dura0ok requested a review from vitcpp August 4, 2023 08:22

dura0ok changed the title ~~[ISSUE #22] Created spoint_deg, scircle_deg functions.~~ [ISSUE #22] Created spoint_deg, scircle_deg, spoly_deg functions. Aug 4, 2023

vitcpp approved these changes Aug 4, 2023

View reviewed changes

esabol approved these changes Aug 4, 2023

View reviewed changes

vitcpp merged commit 7fb5552 into postgrespro:master Aug 4, 2023

esabol mentioned this pull request Aug 5, 2023

Add spoint_deg() and spoly_deg() functions? #22

Closed

[ISSUE #22] Created spoint_deg, scircle_deg, spoly_deg functions. #38

[ISSUE #22] Created spoint_deg, scircle_deg, spoly_deg functions. #38

Uh oh!

Conversation

dura0ok commented Jul 31, 2023

Uh oh!

vitcpp commented Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dura0ok commented Aug 1, 2023

Uh oh!

Uh oh!

dura0ok commented Aug 1, 2023

Uh oh!

esabol commented Aug 2, 2023

Uh oh!

dura0ok commented Aug 2, 2023

Uh oh!

esabol commented Aug 2, 2023

Uh oh!

esabol commented Aug 2, 2023

Uh oh!

dura0ok commented Aug 2, 2023

Uh oh!

vitcpp commented Aug 2, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vitcpp Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dura0ok commented Aug 2, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dura0ok commented Aug 3, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vitcpp Aug 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

esabol left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vitcpp commented Jul 31, 2023 •

edited

Loading