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

SP-GiST index on geometry #258

Closed
wants to merge 20 commits into
base: svn-trunk
from

Conversation

Projects
None yet
5 participants
@Komzpa
Member

Komzpa commented Jun 7, 2018

Follow-up PR from #251

@alesuiss @estebanzimanyi

@Komzpa Komzpa referenced this pull request Jun 7, 2018

Closed

sp-gist index for BOX3D #251

@@ -690,7 +690,7 @@ CREATE OR REPLACE FUNCTION geometry_overright(geom1 geometry, geom2 geometry)
-- Availability: 0.1.0
CREATE OPERATOR &> (
LEFTARG = geometry, RIGHTARG = geometry, PROCEDURE = geometry_overright,
COMMUTATOR = '&<',
-- COMMUTATOR = '&<',

This comment has been minimized.

@Komzpa

Komzpa Jun 7, 2018

Member

@alesuiss this was fixed in trunk, use git merge upstream/svn-trunk

This comment has been minimized.

@alesuiss

alesuiss Jun 7, 2018

Thanks :-)

We did install travis on our fork to test it there, but used PG 9.6 so didn't compile.

@strk

This comment has been minimized.

Member

strk commented Jun 7, 2018

@alesuiss

This comment has been minimized.

alesuiss commented Jun 7, 2018

@strk can you elaborate? What should we do?

@strk

This comment has been minimized.

Member

strk commented Jun 7, 2018

@alesuiss

This comment has been minimized.

alesuiss commented Jun 7, 2018

@strk actually those commutators were fixed upstream by Paul Ramsey, but we also implemented the fix on our side as it was needed for correct results. This lead to tree conflict, which I resolved manually before @Komzpa suggested to simply merge svn-trunk.

@alesuiss

This comment has been minimized.

alesuiss commented Jun 8, 2018

Hi,

FYI, I've been fiddling with Travis to get PG11 working. You can see the branch here and the latest results here.

There are a few outstanding issues:

  • For some reason, it insists on building with clang (3.9), which doesn't recognize -Og
  • One of the tests, "tickets", fails by apparently crashing pg. Curiously, that test passes fine on my workstation. Then, subsequent tests fail because pg is still in recovery mode.
@robe2

This comment has been minimized.

Member

robe2 commented Jul 1, 2018

Is the associated ticket for this - https://trac.osgeo.org/postgis/ticket/1847

@robe2

This comment has been minimized.

Member

robe2 commented Jul 1, 2018

Is #251 already incorporated into this pull request?

@alesuiss

This comment has been minimized.

alesuiss commented Jul 1, 2018

@robe2 This PR is a generalization of #251 to work on the geometry type using the new compress function in PG11.

@robe2

This comment has been minimized.

Member

robe2 commented Jul 1, 2018

@alesuiss @estebanzimanyi need your names and company if any for Credits.

I'm still working on ifdef logic for this . I tested against 11beta2 and all passed.

Some things look like they could go into lower versions, but I'm just going to ifdef all to require PG 11.

@robe2

This comment has been minimized.

Member

robe2 commented Jul 1, 2018

I noticed in postgis/gserialized_gist_2d.c only change was removal of static on a bunch of functions. Is this change related to the purpose of this commit or is it unintentional.

@alesuiss

This comment has been minimized.

alesuiss commented Jul 1, 2018

I noticed in postgis/gserialized_gist_2d.c only change was removal of static on a bunch of functions. Is this change related to the purpose of this commit or is it unintentional.

@robe2 we removed the statics because we need to call these functions for indexes that we are implementing in a separate project. This PR is actually spun off that project.

EDIT: plus we actually call these functions in postgis/gserialized_spgist_2d.c

@robe2

This comment has been minimized.

Member

robe2 commented Jul 1, 2018

@alesuiss okay so I'll leave that out then. If we change to that it would need to come in as a separate pull request and ticket.

What about my other question name for credits?

@alesuiss

This comment has been minimized.

alesuiss commented Jul 1, 2018

What about my other question name for credits?

We are Esteban Zimányi and Arthur Lesuisse from Université Libre de Bruxelles (ULB). Thanks :-)

@robe2

This comment has been minimized.

Member

robe2 commented Jul 1, 2018

I'm going to leave the statics in for now. I'd have to change other parts to make it work if I took it out. @pramsey , @strk or @Komzpa can take it out later if needed

@strk strk closed this in 1d15d5f Jul 1, 2018

@robe2

This comment has been minimized.

Member

robe2 commented Jul 1, 2018

@Komzpa did you say you were going to do the documentation for this?

Normally I don't accept pull requests for new features unless they include user documentation of the feature. In this case since I'm in a rush to release 2.5.0beta1 I did. No super rush on doc. End of July is fine. @alesuiss , @estebanzimanyi feel free to help @Komzpa with the documentation work.

You can pattern it after the GIST and BRIN ones in this section:

https://postgis.net/docs/manual-dev/using_postgis_dbmanagement.html#gist_indexes

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