-
-
Notifications
You must be signed in to change notification settings - Fork 380
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
ST_RemoveIrrelevantPointsForView() - Make optimizations based on cartesian math optional #774
base: master
Are you sure you want to change the base?
ST_RemoveIrrelevantPointsForView() - Make optimizations based on cartesian math optional #774
Conversation
WalkthroughWalkthroughThe changes introduce an optional Changes
Sequence DiagramssequenceDiagram
participant User
participant SQL_Function
participant Internal_Algo
User->>SQL_Function: Call ST_RemoveIrrelevantPointsForView with cartesian_hint
SQL_Function->>Internal_Algo: Pass geometry, bounds, and cartesian_hint
Internal_Algo->>Internal_Algo: Optimize based on cartesian_hint
Internal_Algo-->>SQL_Function: Return optimized points
SQL_Function-->>User: Return result
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
// =============================================================================== | ||
static void removePoints(POINTARRAY *points, GBOX *bounds, bool closed) { | ||
static void removePoints(POINTARRAY *points, GBOX *bounds, bool closed, bool cartesian_hint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why this is not in liblwgeom ? Moving it there would allow adding unit tests for proper memory management debugging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, actually no special reason. What is the difference between postgis and liblwgeom, and are there other files to be adapted when moving lwgeom_remove_irrelevant_points_for_view.c from postgis to liblwgeom folder? (Same question for lwgeom_st_remove_small_parts.c?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@strk When I move the .c file to liblwgeom, adapting postgis/Makefile.in and liblwgeom/Makefile.in I get a lot of compiler errors because of missing import statements. I'm not sure how I could resolve them all. Could you please assist me, or would it be an option to leave the file at its current location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
show the errors ? You mean include statement, I guess. The usefulness of moving the code under liblwgeom is for being able to write unit tests which usually catch more errors, especially memory management related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@strk You are right, I meant include, not import. The errors I get when compiling in /liblwgeom are:
fatal error: postgres.h: No such file or directory
fatal error: funcapi.h: No such file or directory
fatal error: utils/array.h: No such file or directory
fatal error: utils/builtins.h: No such file or directory
fatal error: utils/lsyscache.h: No such file or directory
fatal error: utils/numeric.h: No such file or directory
fatal error: access/htup_details.h: No such file or directory
If I knew how to get rid of these errors I'd try adding cunit tests.
Since IMO there's nothing crucial related to memory management in my code - could we open a new ticket for this refactoring step? I'd like to do the same for ST_RemoveSmallParts. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The functionality needs be split in two.
One part will only touch LWGEOM object and won't need any of those headers.
The other part will be just a thin wrapper for that liblwgeom function, to expose it to SQL, and only that part will need some of those headers (not all, most likely)
// =============================================================================== | ||
static void removePoints(POINTARRAY *points, GBOX *bounds, bool closed) { | ||
static void removePoints(POINTARRAY *points, GBOX *bounds, bool closed, bool cartesian_hint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
show the errors ? You mean include statement, I guess. The usefulness of moving the code under liblwgeom is for being able to write unit tests which usually catch more errors, especially memory management related.
…tps://github.com/gluser1357/postgis-fork.git into remove_irrelevant_points_for_view_cartesian_hint
Pull request for enhancement request https://trac.osgeo.org/postgis/ticket/5744
Please see details there.