HSEARCH-3087 Spatial support for querying and sorting #66

gsmet · 2018-05-29T11:48:37Z

https://hibernate.atlassian.net/browse/HSEARCH-3087

This is the initial support for spatial queries. It contains querying and sorting but does not contain the projection stuff yet.

As for the projection stuff, we either need a projection DSL to be able to define the location or we could only support it for the circle predicate and find a hackyish way to push the information from the query to the projection stuff (not very excited by the latter).

I'm still a bit torn about the API for circle queries: it's more consistent this way but I wonder if user won't search for distance queries or proximity queries rather than having the entities in a circle.

coveralls · 2018-05-29T11:58:16Z

Pull Request Test Coverage Report for Build 488

312 of 341 (91.5%) changed or added relevant lines in 47 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.3%) to 84.174%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
backend-elasticsearch/src/main/java/org/hibernate/search/v6poc/backend/elasticsearch/search/sort/impl/SearchSortFactoryImpl.java	2	3	66.67%
backend-elasticsearch/src/main/java/org/hibernate/search/v6poc/backend/elasticsearch/types/codec/impl/ElasticsearchFieldCodec.java	0	1	0.0%
backend-lucene/src/main/java/org/hibernate/search/v6poc/backend/lucene/search/sort/impl/DistanceSortBuilderImpl.java	10	11	90.91%
backend-lucene/src/main/java/org/hibernate/search/v6poc/backend/lucene/types/codec/impl/GeoPointFieldCodec.java	5	6	83.33%
backend-lucene/src/main/java/org/hibernate/search/v6poc/backend/lucene/types/sort/impl/AbstractStandardLuceneFieldSortContributor.java	2	3	66.67%
backend-lucene/src/main/java/org/hibernate/search/v6poc/backend/lucene/types/sort/impl/GeoPointFieldSortContributor.java	4	5	80.0%
engine/src/main/java/org/hibernate/search/v6poc/search/dsl/sort/impl/DistanceSortContextImpl.java	9	11	81.82%
engine/src/main/java/org/hibernate/search/v6poc/search/dsl/sort/spi/DelegatingSearchSortContainerContextImpl.java	0	2	0.0%
engine/src/main/java/org/hibernate/search/v6poc/spatial/ImmutableGeoBoundingBox.java	12	21	57.14%
engine/src/main/java/org/hibernate/search/v6poc/spatial/ImmutableGeoPolygon.java	26	36	72.22%

Totals
Change from base Build 483:	0.3%
Covered Lines:	8377
Relevant Lines:	9952

💛 - Coveralls

yrodiere

Here are my comments. It's really nice, in particular adding support for bounding boxes and polygons!

As for the projection stuff, we either need a projection DSL to be able to define the location or we could only support it for the circle predicate and find a hackyish way to push the information from the query to the projection stuff (not very excited by the latter).

Right. I created https://hibernate.atlassian.net/browse/HSEARCH-3190 so we don't forget.

I'm still a bit torn about the API for circle queries: it's more consistent this way but I wonder if user won't search for distance queries or proximity queries rather than having the entities in a circle.

We can review that later if you want. Add it to the Search 6 todo document, maybe?

yrodiere · 2018-05-29T14:27:18Z

util/internal/common/src/main/java/org/hibernate/search/v6poc/util/impl/common/Contracts.java

+
+	public static void assertNotNull(Object object, String objectDescription) {
+		if ( object == null ) {
+			throw log.mustNotBeNull( objectDescription );


This method does not exist at this point. You should move the changes to the Log interface from the commit "HSEARCH-3087 Add support for within circle spatial predicates" to this commit.

Right, fixed.

yrodiere · 2018-05-29T14:34:27Z

...te/search/v6poc/backend/lucene/types/sort/impl/AbstractSimpleLuceneFieldSortContributor.java

 import org.hibernate.search.v6poc.search.dsl.sort.SortOrder;

-abstract class AbstractScalarLuceneFieldSortContributor implements LuceneFieldSortContributor {
+abstract class AbstractSimpleLuceneFieldSortContributor implements LuceneFieldSortContributor {


You used the term "standard" to mean "everything but GeoPoint" in StandardFieldPredicateBuilderFactory.
Maybe you could use the same term in both cases? I don't really care if it's "simple" or "standard", but at least let's stick to one.

Right, done.

yrodiere · 2018-05-29T14:38:20Z

.../v6poc/backend/lucene/search/predicate/impl/AbstractSpatialWithinCirclePredicateBuilder.java

+import org.hibernate.search.v6poc.search.predicate.spi.SpatialWithinCirclePredicateBuilder;
+
+
+public abstract class AbstractSpatialWithinCirclePredicateBuilder<T> extends AbstractSearchPredicateBuilder


Not sure I see the point of these abstract classes (AbstractSpatialWithinCirclePredicateBuilder, AbstractSpatialWithinBoundingBoxPredicateBuilder, AbstractSpatialWithinPolygonPredicateBuilder), since they each only have one subclass?

So the point of them is for the hopefully upcoming support of storing polygons or other spatial types in the index.

I'm not sure it is perfectly safe to use exactly the same DSL interfaces and builder interface for those, since there may be parameters that make sense in one case and not another. But ok, let's leave this for now: it's a problem for another day...

Not sure I follow you: if you want to search a polygon in a circle, you will need to define the circle. That's all that is in AbstractSpatialWithinCirclePredicateBuilder.

I am mainly talking about additional, optional parameters. I can imagine searching for polygons within a circle being similar, but slightly different from search for points within a circle.

In particular, when searching for points within a circle, you will use a distance query, which involves picking some distance computation algorithm (arc or plane in Elasticsearch).
When searching for polygons within a circle, you will not use a distance query, at least not in Elasticsearch; you will use a geo_shape query. And this query does not allow you to pick a distance computation algorithm (maybe because it doesn't use one).

As a result, if we decide to expose a .distanceAlgorithm(DistanceAlgorithmEnum) method on the object returned by a call to circle, our API will be dodgy: in some cases ("point" fields) that method can be called, in some other cases ("shape" fields) it cannot.

That's just an example though: looking for points within a circle is not exactly the same as looking for shapes within a circle, so I would expect other differences that could translate into different APIs.

Well, if need be and these classes ends up being useless, we will be able to remove them.

I kinda liked the symmetry with the other types too.

yrodiere · 2018-05-29T14:44:40Z

engine/src/main/java/org/hibernate/search/v6poc/backend/spatial/DistanceUnit.java

+/**
+ * Distance units.
+ */
+public enum DistanceUnit {


We might want to move this and the other classes in this package to another package... e.g. org.hibernate.search.v6poc.spatial.

Up until now it was almost acceptable to have them in a backend-related package, since you basically had to be dealing with low-level interfaces to need GeoPoint and such, but now these classes also appear in the Search DSL, so it's becoming really weird.

yrodiere · 2018-05-29T14:47:39Z

engine/src/main/java/org/hibernate/search/v6poc/backend/spatial/DistanceUnit.java

+/**
+ * Distance units.
+ */
+public enum DistanceUnit {


Did you have a look at JSR-108? From what I recall it's probably too generic, but I may be wrong...

I took a look at this one: https://github.com/unitsofmeasurement .

It's quite complex and I don't think it's worth adding a dependency for that.

Fair enough.

yrodiere · 2018-05-30T14:01:50Z

...oc/backend/lucene/types/predicate/impl/GeoPointSpatialWithinBoundingBoxPredicateBuilder.java

+	@Override
+	protected Query buildQuery() {
+		return LatLonPoint.newBoxQuery( absoluteFieldPath, boundingBox.getBottomRight().getLatitude(), boundingBox.getTopLeft().getLatitude(),
+				boundingBox.getTopLeft().getLatitude(), boundingBox.getBottomRight().getLongitude() );


I see three calls to getLatitude here, is it normal? Also, if the tests didn't catch that, you might want to add some tests.
If it's normal, maybe a comment would help here...

I added a test.

yrodiere · 2018-05-30T14:10:18Z

...java/org/hibernate/search/v6poc/backend/lucene/search/sort/impl/DistanceSortBuilderImpl.java

+
+	@Override
+	public void order(SortOrder order) {
+		// TODO contribute the support of descending order to Lucene


Please create a ticket for that if there isn't one already.

To future me: https://hibernate.atlassian.net/browse/HSEARCH-3193

yrodiere · 2018-05-30T14:20:16Z

...n/java/org/hibernate/search/v6poc/backend/lucene/search/sort/impl/SearchSortFactoryImpl.java

+	public DistanceSortBuilder<LuceneSearchSortCollector> distance(String absoluteFieldPath, GeoPoint location) {
+		LuceneIndexSchemaFieldNode<?> schemaNode = searchTargetModel.getSchemaNode( absoluteFieldPath );
+		if ( !schemaNode.getSortContributor().supportsSortingByDistance() ) {
+			throw log.distanceOperationsNotSupportedByFieldType( absoluteFieldPath );


I don't really like the idea of introducing a spatial-specific getter just for that, but more importantly I don't see the point. For field sorts on geo point fields, we already throw an error late (when calling contribute on the builder), so why wouldn't we do the same for distance sorts on non-geopoint fields, and throw an error when contributeDistanceSort is called?

Alternatively, if you think early failure has value in this case, we could introduce SearchSortBuilderFactories like we do for predicates. In this case that would only require two factories:

a "simple"/"standard" one with a "field sort" factory method that returns a builder and a "distance sort" factory method that throws an error

a "geopoint" one with a "field sort" factory method that throws an error and a "distance sort" factory method that returns a builder

I think that would be more future-proof regardless of whether we want early errors or not, by the way, but that's just my opinion.

Makes sense.

yrodiere · 2018-05-30T14:26:38Z

...ate/search/v6poc/integrationtest/backend/elasticsearch/search/ElasticsearchSearchSortIT.java

+
+	private void initData() {
+		ChangesetIndexWorker<? extends DocumentElement> worker = indexManager.createWorker( sessionContext );
+		// Important: do not index the documents in the expected order after sorts


Well, apparently "first/second/third" is not the expected order after sorts here... Maybe something is wrong?

This message was originally put in a test where we would index in this order: "second", "first", "third"; and then (in the test method) run a search query that would result in this order: "first", "second", "third".
Here the "first", "second", "third" labels do not correspond to anything... Not the indexing order, not the sorted order. So I was just pointing out that maybe the constants should be renamed.

Ah OK :). Fixed then.

yrodiere · 2018-05-30T14:28:35Z

...rc/main/java/org/hibernate/search/v6poc/integrationtest/backend/tck/search/SearchSortIT.java

+		query = simpleQuery( b -> b.byDistance( "geoPoint", 45.757864, 4.834496 ) );
+
+		DocumentReferencesSearchResultAssert.assertThat( query )
+				.hasReferencesHitsExactOrder( indexName, FIRST_ID, THIRD_ID, SECOND_ID, EMPTY_ID );


Maybe comment here, explaining why we don't test descending sorts?

…actStandardLuceneFieldSortContributor

yrodiere · 2018-05-31T12:44:28Z

Merged! Thanks a lot :)

gsmet added the Ready for review label May 29, 2018

yrodiere reviewed May 30, 2018

View reviewed changes

gsmet force-pushed the HSEARCH-3087 branch from a4adfa2 to fee1e95 Compare May 31, 2018 08:37

yrodiere added Changes suggested Ready for review and removed Ready for review Changes suggested labels May 31, 2018

gsmet added 13 commits May 31, 2018 14:10

HSEARCH-3087 Rename AbstractScalarLuceneFieldSortContributor to Abstr…

c3735b1

…actStandardLuceneFieldSortContributor

Add a Contracts utility

22cd370

Various minor cleanups

3e8d1a7

HSEARCH-3087 Add support for within circle spatial predicates

d838522

HSEARCH-3087 Add support for within polygon spatial predicates

f1aa984

HSEARCH-3087 Add support for within bounding box spatial predicates

97415aa

HSEARCH-3087 Support sorting by distance to a given point

ce2b03e

HSEARCH-3087 Enable spatial support in the showcase

28d90ec

HSEARCH-3087 Move the spatial types to their own package

9fd88ff

HSEARCH-3087 Use Collections.addAll() to add points to a polygon

3b8b284

HSEARCH-3087 Make the points of the polygon immutable

adb1f1f

HSEARCH-3087 Push the distance units to the backends

1de5cab

HSEARCH-3087 Force a check that the polygon is properly closed

4f38663

gsmet force-pushed the HSEARCH-3087 branch from 2fe94a2 to 4f38663 Compare May 31, 2018 12:15

yrodiere merged commit 58e0578 into hibernate:master May 31, 2018

		import org.hibernate.search.v6poc.search.predicate.spi.SpatialWithinCirclePredicateBuilder;


		public abstract class AbstractSpatialWithinCirclePredicateBuilder<T> extends AbstractSearchPredicateBuilder

Uh oh!

HSEARCH-3087 Spatial support for querying and sorting #66

HSEARCH-3087 Spatial support for querying and sorting #66

Uh oh!

Conversation

gsmet commented May 29, 2018

Uh oh!

coveralls commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 488

💛 - Coveralls

Uh oh!

yrodiere left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yrodiere commented May 31, 2018

Uh oh!

Uh oh!

coveralls commented May 29, 2018 •

edited

Loading

yrodiere left a comment •

edited

Loading