Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
vitcpp merged 1 commit intopostgrespro:masterfromdura0ok:deg_functions
Aug 4, 2023

Conversation

dura0ok
Copy link

@esabol@vitcpp please, review it.

@vitcpp
Copy link
Contributor

vitcpp commentedJul 31, 2023
edited
Loading

@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!

dura0ok reacted with thumbs up emoji

@dura0okdura0okforce-pushed thedeg_functions branch 3 times, most recently fromaa46131 toe0c76c4CompareAugust 1, 2023 05:30
@dura0okdura0ok requested a review fromesabolAugust 1, 2023 05:30
@dura0okdura0okforce-pushed thedeg_functions branch 2 times, most recently from048e068 to5ccfedbCompareAugust 1, 2023 05:44
@dura0ok
Copy link
Author

@vitcpp I have updated the version

esabol reacted with thumbs up emoji

@dura0ok
Copy link
Author

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 reacted with thumbs up emoji

@esabol
Copy link
Contributor

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 aspoly_deg() for the sake of completeness might be a good idea. 😄

dura0ok reacted with thumbs up emoji

@dura0ok
Copy link
Author

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 aspoly_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
Copy link
Contributor

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

Sorry, I thought I already did. Approved!

@esabol
Copy link
Contributor

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
Copy link
Author

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

esabol reacted with thumbs up emoji

@vitcpp
Copy link
Contributor

@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.

esabol reacted with thumbs up emoji

src/polygon.c Outdated
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])
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

esabol reacted with thumbs up emoji
@dura0okdura0okforce-pushed thedeg_functions branch 3 times, most recently from30a17c7 todf45187CompareAugust 2, 2023 09:53
@dura0okdura0ok requested review fromesabol andvitcppAugust 2, 2023 10:40
@dura0ok
Copy link
Author

I corrected all the comments
Look again, please@esabol

esabol reacted with thumbs up emoji

@dura0okdura0ok requested a review fromesabolAugust 2, 2023 18:11
@dura0ok
Copy link
Author

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

esabol reacted with thumbs up emoji

@dura0okdura0ok requested a review fromesabolAugust 3, 2023 05:08
src/polygon.c Outdated
);
}
PG_RETURN_POINTER(spherepoly_from_array(points, np));

Copy link
Contributor

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.

@dura0okdura0okforce-pushed thedeg_functions branch 2 times, most recently from9d8b007 to4199d35CompareAugust 4, 2023 04:49
@dura0okdura0ok requested a review fromvitcppAugust 4, 2023 05:08
@dura0okdura0ok requested a review fromvitcppAugust 4, 2023 08:22
@dura0okdura0ok changed the title[ISSUE #22] Created spoint_deg, scircle_deg functions.[ISSUE #22] Created spoint_deg, scircle_deg, spoly_deg functions.Aug 4, 2023
Copy link
Contributor

@esabolesabol left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@vitcppvitcpp merged commit7fb5552 intopostgrespro:masterAug 4, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@vitcppvitcppvitcpp approved these changes

@esabolesabolesabol approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@dura0ok@vitcpp@esabol

[8]ページ先頭

©2009-2025 Movatter.jp