diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index a3368ff44a8..9866bbef11e 100644 --- a/lucene/CHANGES.txt +++ b/lucene/CHANGES.txt @@ -232,6 +232,9 @@ Bug Fixes * LUCENE-7112: WeightedSpanTermExtractor.extractUnknownQuery is only called on queries that could not be extracted. (Adrien Grand) +* LUCENE-7126: Remove GeoPointDistanceRangeQuery. This query was implemented + with boolean NOT, and incorrect for multi-valued documents. (Robert Muir) + Other * LUCENE-7035: Upgrade icu4j to 56.1/unicode 8. (Robert Muir) diff --git a/lucene/sandbox/src/test/org/apache/lucene/search/TestLatLonPointQueries.java b/lucene/sandbox/src/test/org/apache/lucene/search/TestLatLonPointQueries.java index 4f2c2d7a23c..3f4da8ad1af 100644 --- a/lucene/sandbox/src/test/org/apache/lucene/search/TestLatLonPointQueries.java +++ b/lucene/sandbox/src/test/org/apache/lucene/search/TestLatLonPointQueries.java @@ -37,11 +37,6 @@ public class TestLatLonPointQueries extends BaseGeoPointTestCase { return LatLonPoint.newDistanceQuery(field, centerLat, centerLon, radiusMeters); } - @Override - protected Query newDistanceRangeQuery(String field, double centerLat, double centerLon, double minRadiusMeters, double radiusMeters) { - return null; - } - @Override protected Query newPolygonQuery(String field, double[] lats, double[] lons) { return LatLonPoint.newPolygonQuery(field, lats, lons); diff --git a/lucene/spatial/src/java/org/apache/lucene/spatial/geopoint/search/GeoPointDistanceRangeQuery.java b/lucene/spatial/src/java/org/apache/lucene/spatial/geopoint/search/GeoPointDistanceRangeQuery.java deleted file mode 100644 index 5cc778a5e7c..00000000000 --- a/lucene/spatial/src/java/org/apache/lucene/spatial/geopoint/search/GeoPointDistanceRangeQuery.java +++ /dev/null @@ -1,122 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.lucene.spatial.geopoint.search; - -import org.apache.lucene.index.IndexReader; -import org.apache.lucene.search.BooleanClause; -import org.apache.lucene.search.BooleanQuery; -import org.apache.lucene.search.Query; -import org.apache.lucene.spatial.geopoint.document.GeoPointField.TermEncoding; - -/** Implements a point distance range query on a GeoPoint field. This is based on - * {@code org.apache.lucene.spatial.geopoint.search.GeoPointDistanceQuery} and is implemented using a - * {@code org.apache.lucene.search.BooleanClause.MUST_NOT} clause to exclude any points that fall within - * minRadiusMeters from the provided point. - * - * @lucene.experimental - */ -public final class GeoPointDistanceRangeQuery extends GeoPointDistanceQuery { - /** minimum distance range (in meters) from lat, lon center location, maximum is inherited */ - protected final double minRadiusMeters; - - /** - * Constructs a query for all {@link org.apache.lucene.spatial.geopoint.document.GeoPointField} types within a minimum / maximum - * distance (in meters) range from a given point - */ - public GeoPointDistanceRangeQuery(final String field, final double centerLat, final double centerLon, - final double minRadiusMeters, final double maxRadiusMeters) { - this(field, TermEncoding.PREFIX, centerLat, centerLon, minRadiusMeters, maxRadiusMeters); - } - - /** - * Constructs a query for all {@link org.apache.lucene.spatial.geopoint.document.GeoPointField} types within a minimum / maximum - * distance (in meters) range from a given point. Accepts an optional - * {@link org.apache.lucene.spatial.geopoint.document.GeoPointField.TermEncoding} - */ - public GeoPointDistanceRangeQuery(final String field, final TermEncoding termEncoding, final double centerLat, final double centerLon, - final double minRadiusMeters, final double maxRadius) { - super(field, termEncoding, centerLat, centerLon, maxRadius); - this.minRadiusMeters = minRadiusMeters; - } - - @Override - public Query rewrite(IndexReader reader) { - Query q = super.rewrite(reader); - if (minRadiusMeters == 0.0) { - return q; - } - - // add an exclusion query - BooleanQuery.Builder bqb = new BooleanQuery.Builder(); - - // create a new exclusion query - GeoPointDistanceQuery exclude = new GeoPointDistanceQuery(field, termEncoding, centerLat, centerLon, minRadiusMeters); - // full map search -// if (radiusMeters >= GeoProjectionUtils.SEMIMINOR_AXIS) { -// bqb.add(new BooleanClause(new GeoPointInBBoxQuery(this.field, -180.0, -90.0, 180.0, 90.0), BooleanClause.Occur.MUST)); -// } else { - bqb.add(new BooleanClause(q, BooleanClause.Occur.MUST)); -// } - bqb.add(new BooleanClause(exclude, BooleanClause.Occur.MUST_NOT)); - - return bqb.build(); - } - - @Override - public String toString(String field) { - final StringBuilder sb = new StringBuilder(); - sb.append(getClass().getSimpleName()); - sb.append(':'); - if (!this.field.equals(field)) { - sb.append(" field="); - sb.append(this.field); - sb.append(':'); - } - return sb.append( " Center: [") - .append(centerLat) - .append(',') - .append(centerLon) - .append(']') - .append(" From Distance: ") - .append(minRadiusMeters) - .append(" m") - .append(" To Distance: ") - .append(radiusMeters) - .append(" m") - .append(" Lower Left: [") - .append(minLat) - .append(',') - .append(minLon) - .append(']') - .append(" Upper Right: [") - .append(maxLat) - .append(',') - .append(maxLon) - .append("]") - .toString(); - } - - /** getter method for minimum distance */ - public double getMinRadiusMeters() { - return this.minRadiusMeters; - } - - /** getter method for maximum distance */ - public double getMaxRadiusMeters() { - return this.radiusMeters; - } -} diff --git a/lucene/spatial/src/test/org/apache/lucene/spatial/geopoint/search/TestGeoPointQuery.java b/lucene/spatial/src/test/org/apache/lucene/spatial/geopoint/search/TestGeoPointQuery.java index 1a739a3e3fd..0f0eaeb555c 100644 --- a/lucene/spatial/src/test/org/apache/lucene/spatial/geopoint/search/TestGeoPointQuery.java +++ b/lucene/spatial/src/test/org/apache/lucene/spatial/geopoint/search/TestGeoPointQuery.java @@ -55,13 +55,6 @@ public class TestGeoPointQuery extends BaseGeoPointTestCase { return new GeoPointDistanceQuery(field, TermEncoding.PREFIX, centerLat, centerLon, radiusMeters); } - @Override - protected Query newDistanceRangeQuery(String field, double centerLat, double centerLon, double minRadiusMeters, double radiusMeters) { - // LUCENE-7126: currently not valid for multi-valued documents, because it rewrites to a BooleanQuery! - // return new GeoPointDistanceRangeQuery(field, TermEncoding.PREFIX, centerLat, centerLon, minRadiusMeters, radiusMeters); - return null; - } - @Override protected Query newPolygonQuery(String field, double[] lats, double[] lons) { return new GeoPointInPolygonQuery(field, TermEncoding.PREFIX, lats, lons); diff --git a/lucene/spatial/src/test/org/apache/lucene/spatial/geopoint/search/TestLegacyGeoPointQuery.java b/lucene/spatial/src/test/org/apache/lucene/spatial/geopoint/search/TestLegacyGeoPointQuery.java index c2f74f16deb..e9b4c1273e8 100644 --- a/lucene/spatial/src/test/org/apache/lucene/spatial/geopoint/search/TestLegacyGeoPointQuery.java +++ b/lucene/spatial/src/test/org/apache/lucene/spatial/geopoint/search/TestLegacyGeoPointQuery.java @@ -55,13 +55,6 @@ public class TestLegacyGeoPointQuery extends BaseGeoPointTestCase { return new GeoPointDistanceQuery(field, TermEncoding.NUMERIC, centerLat, centerLon, radiusMeters); } - @Override - protected Query newDistanceRangeQuery(String field, double centerLat, double centerLon, double minRadiusMeters, double radiusMeters) { - // LUCENE-7126: currently not valid for multi-valued documents, because it rewrites to a BooleanQuery! - // return new GeoPointDistanceRangeQuery(field, TermEncoding.NUMERIC, centerLat, centerLon, minRadiusMeters, radiusMeters); - return null; - } - @Override protected Query newPolygonQuery(String field, double[] lats, double[] lons) { return new GeoPointInPolygonQuery(field, TermEncoding.NUMERIC, lats, lons); diff --git a/lucene/spatial/src/test/org/apache/lucene/spatial/util/BaseGeoPointTestCase.java b/lucene/spatial/src/test/org/apache/lucene/spatial/util/BaseGeoPointTestCase.java index 6ca3318b63c..1c54f4e5222 100644 --- a/lucene/spatial/src/test/org/apache/lucene/spatial/util/BaseGeoPointTestCase.java +++ b/lucene/spatial/src/test/org/apache/lucene/spatial/util/BaseGeoPointTestCase.java @@ -742,8 +742,6 @@ public abstract class BaseGeoPointTestCase extends LuceneTestCase { protected abstract Query newDistanceQuery(String field, double centerLat, double centerLon, double radiusMeters); - protected abstract Query newDistanceRangeQuery(String field, double centerLat, double centerLon, double minRadiusMeters, double radiusMeters); - protected abstract Query newPolygonQuery(String field, double[] lats, double[] lons); static final boolean rectContainsPoint(GeoRect rect, double pointLat, double pointLon) { @@ -769,11 +767,6 @@ public abstract class BaseGeoPointTestCase extends LuceneTestCase { return result; } - static final boolean distanceRangeContainsPoint(double centerLat, double centerLon, double minRadiusMeters, double radiusMeters, double pointLat, double pointLon) { - final double d = SloppyMath.haversinMeters(centerLat, centerLon, pointLat, pointLon); - return d >= minRadiusMeters && d <= radiusMeters; - } - private static abstract class VerifyHits { public void test(AtomicBoolean failed, boolean small, IndexSearcher s, NumericDocValues docIDToID, Set deleted, Query query, double[] lats, double[] lons) throws Exception { @@ -929,13 +922,10 @@ public abstract class BaseGeoPointTestCase extends LuceneTestCase { } else if (random().nextBoolean()) { // Distance - final boolean rangeQuery = random().nextBoolean(); final double centerLat = randomLat(small); final double centerLon = randomLon(small); - double radiusMeters; - double minRadiusMeters; - + final double radiusMeters; if (small) { // Approx 3 degrees lon at the equator: radiusMeters = random().nextDouble() * 333000 + 1.0; @@ -944,36 +934,17 @@ public abstract class BaseGeoPointTestCase extends LuceneTestCase { radiusMeters = random().nextDouble() * GeoUtils.SEMIMAJOR_AXIS * Math.PI / 2.0 + 1.0; } - // generate a random minimum radius between 1% and 95% the max radius - minRadiusMeters = (0.01 + 0.94 * random().nextDouble()) * radiusMeters; - if (VERBOSE) { final DecimalFormat df = new DecimalFormat("#,###.00", DecimalFormatSymbols.getInstance(Locale.ENGLISH)); - System.out.println(" radiusMeters = " + df.format(radiusMeters) - + ((rangeQuery == true) ? " minRadiusMeters = " + df.format(minRadiusMeters) : "")); + System.out.println(" radiusMeters = " + df.format(radiusMeters)); } - try { - if (rangeQuery == true) { - query = newDistanceRangeQuery(FIELD_NAME, centerLat, centerLon, minRadiusMeters, radiusMeters); - } else { - query = newDistanceQuery(FIELD_NAME, centerLat, centerLon, radiusMeters); - } - } catch (IllegalArgumentException e) { - if (e.getMessage().contains("exceeds maxRadius")) { - continue; - } - throw e; - } + query = newDistanceQuery(FIELD_NAME, centerLat, centerLon, radiusMeters); verifyHits = new VerifyHits() { @Override protected boolean shouldMatch(double pointLat, double pointLon) { - if (rangeQuery == false) { - return circleContainsPoint(centerLat, centerLon, radiusMeters, pointLat, pointLon); - } else { - return distanceRangeContainsPoint(centerLat, centerLon, minRadiusMeters, radiusMeters, pointLat, pointLon); - } + return circleContainsPoint(centerLat, centerLon, radiusMeters, pointLat, pointLon); } @Override @@ -981,7 +952,7 @@ public abstract class BaseGeoPointTestCase extends LuceneTestCase { double distanceMeters = SloppyMath.haversinMeters(centerLat, centerLon, pointLat, pointLon); System.out.println(" docID=" + docID + " centerLat=" + centerLat + " centerLon=" + centerLon + " pointLat=" + pointLat + " pointLon=" + pointLon + " distanceMeters=" + distanceMeters - + " vs" + ((rangeQuery == true) ? " minRadiusMeters=" + minRadiusMeters : "") + " radiusMeters=" + radiusMeters); + + " vs radiusMeters=" + radiusMeters); } }; @@ -1206,14 +1177,6 @@ public abstract class BaseGeoPointTestCase extends LuceneTestCase { assertEquals(q1, q2); assertFalse(q1.equals(newDistanceQuery("field2", lat, lon, 10000.0))); - q1 = newDistanceRangeQuery("field", lat, lon, 10000.0, 100000.0); - if (q1 != null) { - // Not all subclasses can make distance range query! - q2 = newDistanceRangeQuery("field", lat, lon, 10000.0, 100000.0); - assertEquals(q1, q2); - assertFalse(q1.equals(newDistanceRangeQuery("field2", lat, lon, 10000.0, 100000.0))); - } - double[] lats = new double[5]; double[] lons = new double[5]; lats[0] = rect.minLat;