Geospatial search by qqmyers · Pull Request #8239 · IQSS/dataverse

qqmyers · 2021-11-11T00:10:15Z

What this PR does / why we need it: Adds bounding box indexing in preparation for providing geosearch or other features

Which issue(s) this PR closes:

Closes #

Special notes for your reviewer:

Suggestions on how to test this:

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Is there a release notes update needed for this change?:

Additional documentation: design doc: https://docs.google.com/document/d/1aQCIM6wbAdgMNLTVSUhvuHQKzWDAvQ5XoYGbfint3P4/edit?usp=sharing

IQSS/8188-cache_version_differences

coveralls · 2021-11-11T00:17:46Z

Coverage increased (+0.008%) to 19.989% when pulling b5383f4 on GlobalDataverseCommunityConsortium:GDCC/geosearch into 11abccf on IQSS:develop.

qqmyers · 2022-07-11T17:40:48Z

Per discussion at the DCM that the improved geospatial indexing could be useful independent of adding further geospatial search/display capabilities, I'm moving this PR from draft to review-ready. Not sure what the Rocky shellspec issue is - probably worth re-running the build to see if there's any real issue.

pdurbin

@qqmyers this looks promising! Can you please merge from develop and add a release note? What about docs? I feel like we should add something...

conf/solr/8.11.1/schema.xml

pdurbin

I added a comment about testing.

pdurbin · 2022-09-12T15:38:58Z

src/main/java/edu/harvard/iq/dataverse/search/IndexServiceBean.java

+                }
            }

            for(String metadataBlockName : metadataBlocksWithValue) {


I rewatched Jim's recent "Experimenting with Geospatial indexing" talk ( https://osf.io/84pnw ) and it reminded me that I asked for an test to be added to SearchIT. I still want this and I'm happy to try to add it, if it make sense for me to jump in the code. 😄

Co-authored-by: Philip Durbin <philipdurbin@gmail.com>

qqmyers · 2022-09-15T23:03:08Z

I'm not sure it is possible to make an IT test yet. The solr query that works is as shown:

In the UI, I don't think we can submit filterqueries and with the API, you can submit the filter query part but I haven't seen how to send the pt and d parameters yet. If that is possible, an IT test would be straight forward.

Otherwise, to test one could use the solr console (dropping the jetty.host-localhost recommended for our solr install to make it accessible, and possibly doing an aws ec2 authorize-security-group-ingress --group-id <id> --protocol tcp --port 8983 --cidr <your ip>/32 to let a browser access the console.)

…mmunityConsortium/dataverse.git into GDCC/geosearch

we may want to test for that, but not here.

With only westLongitude added and the other three coordinates left empty, we were getting the following exception. A null check was added to prevent this. Command [DatasetCreate dataset:132] failed: Exception thrown from bean: javax.ejb.EJBTransactionRolledbackException: Exception thrown from bean: org.apache.solr.client.solrj.impl.HttpSolrClient$RemoteSolrException: Error from server at http://localhost:8983/solr/collection1: ERROR: [doc=dataset_132_draft] Error adding field 'solr_bboxtype'='ENVELOPE(null,null,null,null)' msg=Unable to parse shape given formats "lat,lon", "x y" or as WKT because java.text.ParseException: Expected a number input: ENVELOPE(null,null,null,null)

pdurbin · 2022-10-25T18:52:37Z

@qqmyers ok, but I did go ahead and fix a bug when there was only one coordinate: e5187b2

Next up: names. I haven't heard any objections to geo_point and geo_radius so I'm planning on sticking with them. Unless we want spatial_point and spatial_radius? I'm pretty sure we're limiting ourselves to Earth, though, so I think "geo" is fine (and shorter!). 🌎

On the Solr field side we currently have solr_bboxtype and solr_srpt. None of our existing fields start with solr_ so I'm thinking of lopping that off.

Any reason not to shorted to just bbox? Our Solr fields usually don't end with type and just bbox is what's shown in the docs: https://solr.apache.org/guide/8_11/spatial-search.html#bboxfield

I don't know what to say about RPT. SpatialRecursivePrefixTreeFieldType is a mouthful. I'm not in love with solr_srpt but the only example I see in the docs is location_rpt. Perhaps we should go with that.

Update: In 28215fb I renamed the Solr fields:

solr_srpt is now geolocation
solr_bboxtype is now boundingBox

Feedback welcome!

…#8239

pdurbin · 2022-10-26T21:43:22Z

I added some error checking and updated the docs based on how the code is now.

I talked to @atrisovic about this feature and she immediately asked "Can you sort by distance?" Good question! 😄 I haven't looked into this but maybe? See https://solr.apache.org/guide/8_11/spatial-search.html#distance-sorting-or-boosting-function-queries for sort=geodist() asc. I'd need more data to test with.

I'm also ok with this being API only for now. That's how I wrote it up in the User Guide: https://dataverse-guide--8239.org.readthedocs.build/en/8239/user/find-use-data.html#geospatial-search

I'm ready for review. We could keep hacking but there's lots of other stuff to do as well. 😄 I think this is an excellent toehold into geospatial search!

scolapasta · 2022-10-31T19:28:45Z

Moved back to "This Sprint" so that @pdurbin's recent changes can be reviewed. (@pdurbin reviewed the initial PR and was fine with the changes Jim made.)

sekmiller · 2022-11-16T15:02:50Z

src/test/java/edu/harvard/iq/dataverse/api/SearchIT.java

+        junkRadius.then().assertThat()
+                .statusCode(BAD_REQUEST.getStatusCode())
+                .body("message", CoreMatchers.equalTo("Non-number radius supplied."));



Should we add a test that's actually a valid lat, long and radius?
or a valid lat with a non-numeric long, etc.?

In 364e347 I added a test for if lat/long is too big. It turns out Solr provides a pretty useful error, which was already being bubbled up.

I also took a swing at sending a huge value like "99999999999999999999999999999999" to geo_radius but Solr didn't give an error. I dunno, we already test that this number is positive. I say we let the use send bigger and bigger numbers (these are in kilometers). Eventually, they'll figure out they just aren't going to get a geospatial hit. 😄

sekmiller · 2022-11-16T15:08:47Z

src/main/java/edu/harvard/iq/dataverse/api/Search.java

+
            } catch (Exception ex) {
                return error(Response.Status.BAD_REQUEST, ex.getLocalizedMessage());
            }


Do we want to catch a nonnumeric exception here and give more feedback?

Well, getGeoPoint and getGeoRadius will already throw exceptions with feedback about non-numeric values.

For example, if the user does this:

/api/search?q=*&geo_point=41.9580775,-70.6621063&geo_radius=junk

They'll get this error:

{"status":"ERROR","message":"Non-number radius supplied."}

That said, I may not be following the use cases, the concerns. I'm happy to add more tests for these.

sekmiller

Mainly looked at Phil's most recent changes. Looks good, just a couple of suggestions/questions about testing and error handling.

pdurbin

I'm replying to review feedback.

src/main/java/edu/harvard/iq/dataverse/search/IndexServiceBean.java

pdurbin · 2022-11-21T14:00:36Z

src/main/java/edu/harvard/iq/dataverse/api/Search.java

+
            } catch (Exception ex) {
                return error(Response.Status.BAD_REQUEST, ex.getLocalizedMessage());
            }


Well, getGeoPoint and getGeoRadius will already throw exceptions with feedback about non-numeric values.

For example, if the user does this:

/api/search?q=*&geo_point=41.9580775,-70.6621063&geo_radius=junk

They'll get this error:

{"status":"ERROR","message":"Non-number radius supplied."}

That said, I may not be following the use cases, the concerns. I'm happy to add more tests for these.

pdurbin · 2022-11-21T14:54:14Z

src/test/java/edu/harvard/iq/dataverse/api/SearchIT.java

+        junkRadius.then().assertThat()
+                .statusCode(BAD_REQUEST.getStatusCode())
+                .body("message", CoreMatchers.equalTo("Non-number radius supplied."));



In 364e347 I added a test for if lat/long is too big. It turns out Solr provides a pretty useful error, which was already being bubbled up.

I also took a swing at sending a huge value like "99999999999999999999999999999999" to geo_radius but Solr didn't give an error. I dunno, we already test that this number is positive. I say we let the use send bigger and bigger numbers (these are in kilometers). Eventually, they'll figure out they just aren't going to get a geospatial hit. 😄

sekmiller

Looks good, thanks.

mreekie · 2023-03-30T18:53:06Z

grooming

Looking at the NetCDF deliverables.
This looks like it's a related issue that is related to active work, but was done before we were trying to tag each issue
Adding it to the backlog as cleared, and tagging it.

qqmyers added 2 commits September 9, 2021 17:47

add bonding box indexing

f74e0c2

Merge remote-tracking branch 'IQSS/develop' into

f3db9ee

IQSS/8188-cache_version_differences

qqmyers added 4 commits February 2, 2022 13:59

Merge remote-tracking branch 'IQSS/develop' into GDCC/geosearch

7b15877

Merge remote-tracking branch 'IQSS/develop' into GDCC/geosearch

d9eed2f

Merge remote-tracking branch 'IQSS/develop' into GDCC/geosearch

93a3cb6

Merge remote-tracking branch 'IQSS/develop' into GDCC/geosearch

84e9614

qqmyers marked this pull request as ready for review July 11, 2022 17:38

qqmyers added 5 commits July 28, 2022 17:59

Merge remote-tracking branch 'IQSS/develop' into GDCC/geosearch

e765a8d

Merge remote-tracking branch 'IQSS/develop' into GDCC/geosearch

d5430e4

Merge remote-tracking branch 'IQSS/develop' into GDCC/geosearch

4ec3d0f

Merge remote-tracking branch 'IQSS/develop' into GDCC/geosearch

6da3b2a

Merge remote-tracking branch 'IQSS/develop' into GDCC/geosearch

3b8c859

pdurbin self-assigned this Sep 8, 2022

pdurbin reviewed Sep 8, 2022

View reviewed changes

conf/solr/8.11.1/schema.xml Outdated Show resolved Hide resolved

conf/solr/8.11.1/schema.xml Show resolved Hide resolved

conf/solr/8.11.1/schema.xml Show resolved Hide resolved

pdurbin assigned qqmyers Sep 8, 2022

pdurbin reviewed Sep 12, 2022

View reviewed changes

qqmyers and others added 2 commits September 14, 2022 13:46

Merge remote-tracking branch 'IQSS/develop' into GDCC/geosearch

8ce67a3

Update conf/solr/8.11.1/schema.xml

1007a6b

Co-authored-by: Philip Durbin <philipdurbin@gmail.com>

qqmyers and others added 9 commits September 15, 2022 19:19

release note and addition to search doc

b903e2a

Merge branch 'GDCC/geosearch' of https://github.com/GlobalDataverseCo…

d79ba87

…mmunityConsortium/dataverse.git into GDCC/geosearch

add space

c210821

Merge branch 'develop' into GDCC/geosearch IQSS#8239

8c11b7d

Merge branch 'develop' into GDCC/geosearch IQSS#8239

de079fd

add multivalued in schema

0b0c3b9

Merge branch 'GDCC/geosearch' of https://github.com/GlobalDataverseCo…

23a6d58

…mmunityConsortium/dataverse.git into GDCC/geosearch

case matters

cf9b4ae

north < south latitude is an error

f542925

we may want to test for that, but not here.

pdurbin added 3 commits October 25, 2022 16:14

rename solr_srpt to geolocation and solr_bboxtype to boundingBox IQSS…

28215fb

…#8239

add error checking for geo_point and geo_radius IQSS#8239

ff32672

update docs and release note (supported via API) IQSS#8239

6e7499e

pdurbin changed the title ~~GDCC/Geosearch indexing~~ Geospatial search Oct 26, 2022

pdurbin removed their assignment Oct 26, 2022

Merge remote-tracking branch 'IQSS/develop' into GDCC/geosearch

b5383f4

sekmiller self-assigned this Nov 15, 2022

sekmiller reviewed Nov 16, 2022

View reviewed changes

pdurbin self-assigned this Nov 18, 2022

pdurbin added 2 commits November 21, 2022 09:14

test invalid lat/long (too large) IQSS#8239

364e347

Merge branch 'develop' into GDCC/geosearch IQSS#8239

9d8332d

pdurbin reviewed Nov 21, 2022

View reviewed changes

sekmiller approved these changes Nov 22, 2022

View reviewed changes

sekmiller unassigned pdurbin and sekmiller Nov 22, 2022

kcondon self-assigned this Nov 30, 2022

kcondon merged commit b62a8a5 into IQSS:develop Nov 30, 2022

pdurbin added this to the 5.13 milestone Dec 1, 2022

jggautier mentioned this pull request Mar 2, 2023

Dataset publishing prevented or slowed by errors related to possible validation of metadata entered in Geospatial Bounding Box fields #9421

Closed

mreekie added the pm.netcdf-hdf5.d All 3 aims are currently under this deliverable label Mar 30, 2023

pdurbin mentioned this pull request May 30, 2023

Add validation for entered coordinate values in Geospatial metadata (bounding boxes, etc) #9547

Closed

cmbz added FY26 Sprint 14 FY26 Sprint 14 (2025-12-31 - 2026-01-14) and removed FY26 Sprint 14 FY26 Sprint 14 (2025-12-31 - 2026-01-14) labels Jan 14, 2026

Comments

Conversation

qqmyers commented Nov 11, 2021 • edited by pdurbin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qqmyers commented Jul 11, 2022

Uh oh!

pdurbin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pdurbin left a comment

Choose a reason for hiding this comment

Uh oh!

pdurbin Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

qqmyers commented Sep 15, 2022

Uh oh!

pdurbin commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pdurbin commented Oct 26, 2022

Uh oh!

scolapasta commented Oct 31, 2022

Uh oh!

sekmiller Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

pdurbin Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

sekmiller Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

pdurbin Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

sekmiller left a comment

Choose a reason for hiding this comment

Uh oh!

pdurbin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pdurbin Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

pdurbin Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

sekmiller left a comment

Choose a reason for hiding this comment

Uh oh!

mreekie commented Mar 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

qqmyers commented Nov 11, 2021 •

edited by pdurbin

Loading

coveralls commented Nov 11, 2021 •

edited

Loading

pdurbin commented Oct 25, 2022 •

edited

Loading