Skip to content

Commit

Permalink
issue #2879 - revamp radius-based distance calculation
Browse files Browse the repository at this point in the history 
I don't think this was ever working right. Now I think it at least
follows the formulas laid out at

Depends on the RDBMS supporting the `RADIANS` function (which supposedly
works the same across at least Derby, Db2, and Postgresql).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
  • Loading branch information
@lmsurpre
lmsurpre committed Oct 21, 2021
1 parent 5a71eb4 commit bf8983a
Show file tree
Hide file tree
Showing 10 changed files with 174 additions and 71 deletions.
21 changes: 21 additions & 0 deletions fhir-database-utils/src/main/java/com/ibm/fhir/database/utils/query/BaseWhereAdapter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.ibm.fhir.database.utils.query.node.LongBindMarkerNode;
import com.ibm.fhir.database.utils.query.node.NotExistsExpNode;
import com.ibm.fhir.database.utils.query.node.PredicateParser;
import com.ibm.fhir.database.utils.query.node.RadiansExpNode;
import com.ibm.fhir.database.utils.query.node.SelectExpNode;
import com.ibm.fhir.database.utils.query.node.SinExpNode;
import com.ibm.fhir.database.utils.query.node.StringBindMarkerNode;
Expand Down Expand Up @@ -823,6 +824,26 @@ public T acos(ColumnRef arg) {
return getThis();
}

/**
* Add RADIANS(arg) to the expression
* @param arg
* @return
*/
public T radians(ExpNode arg) {
predicateParser.addToken(new RadiansExpNode(arg));
return getThis();
}

/**
* Add RADIANS(arg) to the expression
* @param arg
* @return
*/
public T radians(ColumnRef arg) {
predicateParser.addToken(new RadiansExpNode(new ColumnExpNode(arg.getRef())));
return getThis();
}

/**
* Add an IS NOT NULL operator to the expression
* @return
Expand Down
5 changes: 5 additions & 0 deletions ...tils/src/main/java/com/ibm/fhir/database/utils/query/expression/ColumnExpNodeVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,4 +228,9 @@ public Set<String> acos(Set<String> arg) {
public Set<String> sin(Set<String> arg) {
return arg;
}

@Override
public Set<String> radians(Set<String> arg) {
return arg;
}
}
9 changes: 9 additions & 0 deletions ...tils/src/main/java/com/ibm/fhir/database/utils/query/expression/StringExpNodeVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,15 @@ public String sin(String arg) {
return result.toString();
}

@Override
public String radians(String arg) {
StringBuilder result = new StringBuilder();
result.append("RADIANS(");
result.append(arg);
result.append(")");
return result.toString();
}

/**
* Make sure any literal strings we're embedding in our SQL statements are safe. String parameters
* should almost always use bind variables (which don't require encoding).
Expand Down
7 changes: 7 additions & 0 deletions fhir-database-utils/src/main/java/com/ibm/fhir/database/utils/query/node/ExpNodeVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,4 +90,11 @@ public interface ExpNodeVisitor<T> {
* @return
*/
T sin(T arg);

/**
* SQL RADIANS function
* @param arg
* @return
*/
T radians(T arg);
}
45 changes: 45 additions & 0 deletions fhir-database-utils/src/main/java/com/ibm/fhir/database/utils/query/node/RadiansExpNode.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* (C) Copyright IBM Corp. 2021
*
* SPDX-License-Identifier: Apache-2.0
*/

package com.ibm.fhir.database.utils.query.node;

import java.util.Stack;

/**
* The RADIANS SQL function
*/
public class RadiansExpNode implements ExpNode {
private final ExpNode arg;

/**
* Public constructor
* @param refs
*/
public RadiansExpNode(ExpNode arg) {
this.arg = arg;
}

@Override
public <T> T visit(ExpNodeVisitor<T> visitor) {
return visitor.radians(arg.visit(visitor));
}

@Override
public int precedence() {
return 5;
}

@Override
public void popOperands(Stack<ExpNode> stack) {
// NOP
}

@Override
public boolean isOperand() {
// behave like an operand in the expression tree
return true;
}
}
31 changes: 17 additions & 14 deletions ...bc/src/main/java/com/ibm/fhir/persistence/jdbc/util/type/NewLocationParmBehaviorUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
* variables.
*/
public class NewLocationParmBehaviorUtil {
// the radius of the earth in km (using a spherical approximation)
private static int R = 6371;

/**
* build location search query based on the bounding areas.
*
Expand Down Expand Up @@ -87,6 +90,7 @@ public void buildLocationSearchQuery(WhereFragment whereClauseSegment,
* @param whereClauseSegment
* @param missingArea
*/
@Deprecated
public void buildQueryForBoundingMissing(WhereFragment whereClauseSegment,
BoundingMissing missingArea) {
// No Operation - the main logic is contained in the process Missing parameter
Expand Down Expand Up @@ -129,9 +133,13 @@ public void buildQueryForBoundingBox(WhereFragment whereClauseSegment,
public void buildQueryForBoundingRadius(WhereFragment whereClauseSegment, String paramAlias,
BoundingRadius boundingRadius) {
// This section of code is based on code from http://janmatuschek.de/LatitudeLongitudeBoundingCoordinates
// ACOS(SIN(boundingRadiusLatitude) * SIN(LATITUDE_VALUE) + COS(boundingRadiusLatitude) * COS(LATITUDE_VALUE) * COS(LONGITUDE_VALUE)
WhereFragment arcRadius = new WhereFragment();
arcRadius
// ACOS(SIN(boundingRadiusLatitude) * SIN(LATITUDE_VALUE) + COS(boundingRadiusLatitude) * COS(LATITUDE_VALUE) * COS(boundingRadiusLongitude - LONGITUDE_VALUE) * R

// First, build the subtraction expression
WhereFragment longitudeDiff = new WhereFragment().bind(boundingRadius.getLongitude()).sub().col(paramAlias, LONGITUDE_VALUE);

// Then, use it to build the expression that gets passed to ACOS
WhereFragment arcRadius = new WhereFragment()
.sin(bind(boundingRadius.getLatitude()))
.mult()
.sin(col(paramAlias, LATITUDE_VALUE))
Expand All @@ -140,18 +148,13 @@ public void buildQueryForBoundingRadius(WhereFragment whereClauseSegment, String
.mult()
.cos(col(paramAlias, LATITUDE_VALUE))
.mult()
.cos(col(paramAlias, LONGITUDE_VALUE));
.cos(longitudeDiff.getExpression());

// Finally, put it all together
whereClauseSegment.leftParen()
.col(paramAlias, LATITUDE_VALUE).lte().bind(boundingRadius.getLatitude())
.and()
.col(paramAlias, LATITUDE_VALUE).gte().bind(boundingRadius.getLatitude())
.and()
.col(paramAlias, LONGITUDE_VALUE).lte().bind(boundingRadius.getLongitude())
.and()
.col(paramAlias, LONGITUDE_VALUE).gte().bind(boundingRadius.getLongitude())
// Check the ARC Radius
.and().acos(arcRadius.getExpression()).lte().bind(boundingRadius.getRadius());
whereClauseSegment.rightParen();
.acos(arcRadius.getExpression()).mult().literal(R)
.lte()
.bind(boundingRadius.getRadius())
.rightParen();
}
}
12 changes: 11 additions & 1 deletion ...ence-jdbc/src/test/java/com/ibm/fhir/persistence/jdbc/search/test/JDBCSearchNearTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.List;
import java.util.Map;
import java.util.Properties;
import java.util.UUID;
import java.util.logging.LogManager;

import org.testng.annotations.AfterClass;
Expand All @@ -35,6 +36,8 @@
import com.ibm.fhir.model.resource.Location;
import com.ibm.fhir.model.resource.Resource;
import com.ibm.fhir.model.test.TestUtil;
import com.ibm.fhir.model.type.Id;
import com.ibm.fhir.model.type.Instant;
import com.ibm.fhir.persistence.FHIRPersistence;
import com.ibm.fhir.persistence.MultiResourceResult;
import com.ibm.fhir.persistence.SingleResourceResult;
Expand Down Expand Up @@ -95,13 +98,20 @@ public void startup() throws Exception {
}

savedResource = TestUtil.readExampleResource("json/spec/location-example.json");
savedResource = savedResource.toBuilder()
.id(UUID.randomUUID().toString())
.meta(savedResource.getMeta().toBuilder()
.lastUpdated(Instant.now())
.versionId(Id.of("1"))
.build())
.build();

ICommonTokenValuesCache rrc = new CommonTokenValuesCacheImpl(100, 100, 100);
FHIRPersistenceJDBCCache cache = new FHIRPersistenceJDBCCacheImpl(new NameIdCache<Integer>(), new IdNameCache<Integer>(), new NameIdCache<Integer>(), rrc);
persistence = new FHIRPersistenceJDBCImpl(this.testProps, connectionPool, cache);

SingleResourceResult<Location> result =
persistence.create(FHIRPersistenceContextFactory.createPersistenceContext(null), savedResource);
persistence.createWithMeta(FHIRPersistenceContextFactory.createPersistenceContext(null), savedResource);
assertTrue(result.isSuccess());
assertNotNull(result.getResource());
savedResource = result.getResource();
Expand Down
50 changes: 34 additions & 16 deletions ...c/src/test/java/com/ibm/fhir/persistence/jdbc/test/util/LocationParmBehaviorUtilTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,13 +131,16 @@ public void testBoundingBox() throws FHIRPersistenceException {
String expectedSql =
"(pX.LATITUDE_VALUE >= ? AND pX.LATITUDE_VALUE <= ? AND pX.LONGITUDE_VALUE >= ? AND pX.LONGITUDE_VALUE <= ?)";

BoundingBox boundingBox =
BoundingBox.builder().maxLatitude(10.0).minLatitude(-10.0).maxLongitude(20.0).minLongitude(-20.0)
.build();
BoundingBox boundingBox = BoundingBox.builder()
.maxLatitude(10.0)
.minLatitude(-10.0)
.maxLongitude(20.0)
.minLongitude(-20.0)
.build();
runTest(expectedBindVariables, expectedSql, boundingBox);
}

@Test(expectedExceptions = {})
@Test
public void testBoundingRadius() throws FHIRPersistenceException {
List<Object> expectedBindVariables = new ArrayList<>();
expectedBindVariables.add(10.0);
Expand All @@ -159,9 +162,12 @@ public void testBoundingRadius() throws FHIRPersistenceException {
@Test
public void testBoundingList() throws FHIRPersistenceException {
BoundingRadius boundingRadius = BoundingRadius.builder().latitude(10.0).longitude(21.0).radius(4.0).build();
BoundingBox boundingBox =
BoundingBox.builder().maxLatitude(11.0).minLatitude(-10.0).maxLongitude(20.0).minLongitude(-20.0)
.build();
BoundingBox boundingBox = BoundingBox.builder()
.maxLatitude(11.0)
.minLatitude(-10.0)
.maxLongitude(20.0)
.minLongitude(-20.0)
.build();

List<Bounding> boundingAreas = Arrays.asList(boundingRadius, boundingBox);

Expand Down Expand Up @@ -189,18 +195,30 @@ public void testBoundingList() throws FHIRPersistenceException {

@Test
public void testBoundingMultipleParmsList() throws FHIRPersistenceException {
BoundingBox boundingBox1 =
BoundingBox.builder().maxLatitude(11.0).minLatitude(-10.0).maxLongitude(20.0).minLongitude(-20.0)
.build();
BoundingBox boundingBox1 = BoundingBox.builder()
.maxLatitude(11.0)
.minLatitude(-10.0)
.maxLongitude(20.0)
.minLongitude(-20.0)
.build();
boundingBox1.setInstance(0);
BoundingBox boundingBox2 =
BoundingBox.builder().maxLatitude(11.0).minLatitude(-10.0).maxLongitude(20.0).minLongitude(-20.0)
.build();

BoundingBox boundingBox2 = BoundingBox.builder()
.maxLatitude(11.0)
.minLatitude(-10.0)
.maxLongitude(20.0)
.minLongitude(-20.0)
.build();
boundingBox2.setInstance(0);
BoundingBox boundingBox3 =
BoundingBox.builder().maxLatitude(11.0).minLatitude(-10.0).maxLongitude(20.0).minLongitude(-20.0)
.build();

BoundingBox boundingBox3 = BoundingBox.builder()
.maxLatitude(11.0)
.minLatitude(-10.0)
.maxLongitude(20.0)
.minLongitude(-20.0)
.build();
boundingBox3.setInstance(1);

BoundingMissing boundingBox4 = new BoundingMissing();
boundingBox4.setInstance(2);

Expand Down
55 changes: 20 additions & 35 deletions fhir-search/src/main/java/com/ibm/fhir/search/location/NearLocationHandler.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* (C) Copyright IBM Corp. 2019
* (C) Copyright IBM Corp. 2019, 2021
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand Down Expand Up @@ -62,26 +62,6 @@ public NearLocationHandler() {
}
}

/**
* degrees to radians
*
* @param deg
* @return
*/
public double degree2radians(double deg) {
return Math.PI * deg / 180.0;
}

/**
* radians to degrees
*
* @param rad
* @return
*/
public double radians2degrees(double rad) {
return 180.0 * rad / Math.PI;
}

/**
* build a bounding box given a latitude, longitude and distance.
* <br>
Expand Down Expand Up @@ -123,8 +103,8 @@ public BoundingBox createBoundingBox(double latitude, double longitude, double d
// Convert to Radians to do the ARC calculation
// Based on https://stackoverflow.com/a/238558/1873438
// Verified at https://www.movable-type.co.uk/scripts/latlong.html
double latRad = degree2radians(latitude);
double lonRad = degree2radians(longitude);
double latRad = Math.toRadians(latitude);
double lonRad = Math.toRadians(longitude);

// build bounding box points
// The max/min ensures we don't loop infinitely over the pole, and are not taking silly.
Expand All @@ -135,15 +115,18 @@ public BoundingBox createBoundingBox(double latitude, double longitude, double d
double lonMax = lonRad + distance / (RADIUS_MERIDIAN * Math.cos(latRad));

// Convert back to degrees, and minimize the box.
minLatitude = Math.max(-90, radians2degrees(latMin));
maxLatitude = Math.min(90, radians2degrees(latMax));
minLongitude = Math.max(-180, radians2degrees(lonMin));
maxLongitude = Math.min(180, radians2degrees(lonMax));
minLatitude = Math.max(-90, Math.toDegrees(latMin));
maxLatitude = Math.min(90, Math.toDegrees(latMax));
minLongitude = Math.max(-180, Math.toDegrees(lonMin));
maxLongitude = Math.min(180, Math.toDegrees(lonMax));
}

BoundingBox boundingBox =
BoundingBox.builder().minLatitude(minLatitude).maxLatitude(maxLatitude).minLongitude(minLongitude)
.maxLongitude(maxLongitude).build();
BoundingBox boundingBox = BoundingBox.builder()
.minLatitude(minLatitude)
.maxLatitude(maxLatitude)
.minLongitude(minLongitude)
.maxLongitude(maxLongitude)
.build();

if (logger.isLoggable(Level.FINE)) {
logger.fine("distance: [" + convertedDistance + "] km, original unit: [" + unit + "]");
Expand Down Expand Up @@ -214,12 +197,12 @@ public List<Bounding> generateLocationPositionsFromParameters(List<QueryParamete
longitude = Double.parseDouble(components[1]);

// Distance is included
if (components.length >= 3) {
if (components.length > 2) {
distance = Double.parseDouble(components[2]);
}

// The user has set the units value.
if (components.length >= 3) {
if (components.length > 3) {
unit = components[3];
}
} catch (NumberFormatException | NullPointerException e) {
Expand Down Expand Up @@ -274,9 +257,11 @@ public Bounding createBoundingRadius(double latitude, double longitude, double d
"Invalid unit: '" + unit + "'. Must a UOM length.");
}

BoundingRadius boundingRadiusObj =
BoundingRadius.builder().latitude(latitude).longitude(longitude).radius(convertedDistance)
.build();
BoundingRadius boundingRadiusObj = BoundingRadius.builder()
.latitude(latitude)
.longitude(longitude)
.radius(convertedDistance)
.build();

if (logger.isLoggable(Level.FINE)) {
logger.fine("distance: [" + convertedDistance + "] km, original unit: [" + unit + "]");
Expand Down
Loading

0 comments on commit bf8983a

Please sign in to comment.