I almost think that org.hibernate.search.spatial.SpatialQueryBuilder should be an internal class. Do we want to offer direct access to these instead of the dsl? The answer could be yes, but I'd like to see a use case. On 9 oct. 2012, at 18:45, Sanne Grinovero <sanne(a)hibernate.org&gt; wrote: > Hi Nicolas, > > In the QueryBuilder DSL a spatial Query has a nice option to define > which units are being used: > > org.apache.lucene.search.Query luceneQuery = > builder.spatial().onCoordinates( UserRange.class.getName() ) > .within( 50, Unit.KM ).ofLatitude( centerLatitude ).andLongitude( > centerLongitude ).createQuery(); > > > org.hibernate.search.spatial.SpatialQueryBuilder.buildSpatialQueryByGrid(double, > double, double, String) > has a javadoc comment specifying the parameters are expected to be KM. > > I guess we should pick a strategy and be consistent with it; I think > we should add the Unit parameter to the SpatialQueryBuilder; > > Any thoughts about it? Can I assume you'll be able to look into that? > > Cheers, > Sanne > > Tracked by https://hibernate.onjira.com/browse/HSEARCH-1203 > _______________________________________________ > hibernate-dev mailing list > hibernate-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev

I see a few options: 1. Make onCoordinates optional For me that does not look right and would make the dsl confusing but I'd like to see additional feedback 2. Use a constant .onCoordinates(Statial.DEFAULT_COORDINATES_PROPERTY) 3. Use the class name ( that's what you did ) 4. Use "" as the value .onCoordinates("") 5. Add a specific method .onDefaultCoordinates() So far 5 seems the most natural to me. Emmanuel On 10 oct. 2012, at 09:20, Nicolas Helleringer < nicolas.helleringer(a)gmail.com&gt; wrote: This makes sense. The DSL is clearly THE way to build spatial queries. It simple and elegant. By the way, Emmanuel, I would like some help to remove the .onCoordinates( PoI.class.getName() ) in Query luceneQuery = builder.spatial() .onCoordinates( PoI.class.getName() ) .within( 50, Unit.KM ) .ofLatitude( centerLatitude ) .andLongitude( centerLongitude ) .createQuery(); when building a spatial query on a class with a @Spatial that does not have a name attribute set and thus using the default value of class.getName() Niko 2012/10/10 Emmanuel Bernard <emmanuel(a)hibernate.org&gt; > I almost think that org.hibernate.search.spatial.SpatialQueryBuilder > should be an internal class. Do we want to offer direct access to these > instead of the dsl? > The answer could be yes, but I'd like to see a use case. > > On 9 oct. 2012, at 18:45, Sanne Grinovero <sanne(a)hibernate.org&gt; wrote: > > > Hi Nicolas, > > > > In the QueryBuilder DSL a spatial Query has a nice option to define > > which units are being used: > > > > org.apache.lucene.search.Query luceneQuery = > > builder.spatial().onCoordinates( UserRange.class.getName() ) > > .within( 50, Unit.KM ).ofLatitude( centerLatitude > ).andLongitude( > > centerLongitude ).createQuery(); > > > > > > > org.hibernate.search.spatial.SpatialQueryBuilder.buildSpatialQueryByGrid(double, > > double, double, String) > > has a javadoc comment specifying the parameters are expected to be KM. > > > > I guess we should pick a strategy and be consistent with it; I think > > we should add the Unit parameter to the SpatialQueryBuilder; > > > > Any thoughts about it? Can I assume you'll be able to look into that? > > > > Cheers, > > Sanne > > > > Tracked by https://hibernate.onjira.com/browse/HSEARCH-1203 > > _______________________________________________ > > hibernate-dev mailing list > > hibernate-dev(a)lists.jboss.org > > https://lists.jboss.org/mailman/listinfo/hibernate-dev >

The default name for @Spatial is the empty string (at least as defined on the annotation before we override it to the entity mode). Considering we have @Spatials (plural) to support multiple coordinates on a single entity, would it make sense to support onCoordinates(String... names) ? This has the nice effect of supporting the method without any parameter, so one can use Query luceneQuery = builder.spatial() .onCoordinates() .within( 50, Unit.KM ) ... to point to the default one (undefined, aka empty string) Can also target multiple constraints at once: Query luceneQuery = builder.spatial() .onCoordinates( "home", "office" ) .within( 50, Unit.KM ) ... Looks not so good when mixing default and specific: Query luceneQuery = builder.spatial() .onCoordinates( "", "office" ) .within( 50, Unit.KM ) ... But my guess is that when having multiple coordinates on an entity it should be recommended to name them all explicitly; we could even require this. -- +1 to not consider "SpatialQueryBuilder.buildSpatialQueryByGrid" a public API but in that case I think we should update the tests using it to use the DSL.

Sanne On 10 October 2012 08:44, Nicolas Helleringer <nicolas.helleringer(a)gmail.com&gt; wrote: > 5 seems just fine to me > > Niko > > 2012/10/10 Emmanuel Bernard <emmanuel(a)hibernate.org&gt; >> >> I see a few options: >> >> 1. Make onCoordinates optional >> >> For me that does not look right and would make the dsl confusing but I'd >> like to see additional feedback >> >> 2. Use a constant >> >> .onCoordinates(Statial.DEFAULT_COORDINATES_PROPERTY) >> >> 3. Use the class name ( that's what you did ) >> >> 4. Use "" as the value >> >> .onCoordinates("") >> >> 5. Add a specific method >> >> .onDefaultCoordinates() >> >> So far 5 seems the most natural to me. >> >> Emmanuel >> >> >> On 10 oct. 2012, at 09:20, Nicolas Helleringer >> <nicolas.helleringer(a)gmail.com&gt; wrote: >> >> This makes sense. >> The DSL is clearly THE way to build spatial queries. It simple and >> elegant. >> >> By the way, Emmanuel, I would like some help to remove the >> .onCoordinates( PoI.class.getName() ) >> >> in >> >> Query luceneQuery = builder.spatial() >> .onCoordinates( PoI.class.getName() ) >> .within( 50, Unit.KM ) >> .ofLatitude( centerLatitude ) >> .andLongitude( centerLongitude ) >> .createQuery(); >> >> when building a spatial query on a class with a @Spatial that does not >> have a name attribute set and thus using the default value of >> class.getName() >> >> Niko >> >> >> 2012/10/10 Emmanuel Bernard <emmanuel(a)hibernate.org&gt; >>> >>> I almost think that org.hibernate.search.spatial.SpatialQueryBuilder >>> should be an internal class. Do we want to offer direct access to these >>> instead of the dsl? >>> The answer could be yes, but I'd like to see a use case. >>> >>> On 9 oct. 2012, at 18:45, Sanne Grinovero <sanne(a)hibernate.org&gt; wrote: >>> >>> > Hi Nicolas, >>> > >>> > In the QueryBuilder DSL a spatial Query has a nice option to define >>> > which units are being used: >>> > >>> > org.apache.lucene.search.Query luceneQuery = >>> > builder.spatial().onCoordinates( UserRange.class.getName() ) >>> > .within( 50, Unit.KM ).ofLatitude( centerLatitude >>> > ).andLongitude( >>> > centerLongitude ).createQuery(); >>> > >>> > >>> > >>> > org.hibernate.search.spatial.SpatialQueryBuilder.buildSpatialQueryByGrid(double, >>> > double, double, String) >>> > has a javadoc comment specifying the parameters are expected to be KM. >>> > >>> > I guess we should pick a strategy and be consistent with it; I think >>> > we should add the Unit parameter to the SpatialQueryBuilder; >>> > >>> > Any thoughts about it? Can I assume you'll be able to look into that? >>> > >>> > Cheers, >>> > Sanne >>> > >>> > Tracked by https://hibernate.onjira.com/browse/HSEARCH-1203 >>> > _______________________________________________ >>> > hibernate-dev mailing list >>> > hibernate-dev(a)lists.jboss.org >>> > https://lists.jboss.org/mailman/listinfo/hibernate-dev >> >> >

> > onCoordinates(String... names) Does `onCoordinates()` means all coordinates or simply the default one. If all, then existing queries will break if you add an new coordinate property to an entity.

Also, I'm not sure there is a use case for targeting in the same query, the various coordinates defined on a given entity. But I have no real idea.

Otherwise, I'm fairly neutral to the proposal.

> > +1 to not consider > > "SpatialQueryBuilder.buildSpatialQueryByGrid" a public API but in that > > case I think we should update the tests using it to use the DSL. > > About this, we should also not mention it in the docs: we're using it > in most examples, excluding only the paragraph introducing the Spacial > specific DSL. > We also refer to it frequently in 9.6, the low level details > explanation chapter. Damn, I thought we discussed that already to remove it from the doc. But maybe it was for the unit tests. If we go for the private route, we need to move those classes to an `impl` package. What's weird is that I thought I did it in the past :/

> > onCoordinates(String... names) Does `onCoordinates()` means all coordinates or simply the default one. If all, then existing queries will break if you add an new coordinate property to an entity. Also, I'm not sure there is a use case for targeting in the same query, the various coordinates defined on a given entity. But I have no real idea. Otherwise, I'm fairly neutral to the proposal. > > +1 to not consider > > "SpatialQueryBuilder.buildSpatialQueryByGrid" a public API but in that > > case I think we should update the tests using it to use the DSL. > > About this, we should also not mention it in the docs: we're using it > in most examples, excluding only the paragraph introducing the Spacial > specific DSL. > We also refer to it frequently in 9.6, the low level details > explanation chapter. Damn, I thought we discussed that already to remove it from the doc. But maybe it was for the unit tests. If we go for the private route, we need to move those classes to an `impl` package. What's weird is that I thought I did it in the past :/