issue #2879 - revamp radius-based distance calculation

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>

fhir-database-utils/src/main/java/com/ibm/fhir/database/utils/query/BaseWhereAdapter.java

@@ -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;
@@ -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

fhir-database-utils/src/main/java/com/ibm/fhir/database/utils/query/expression/ColumnExpNodeVisitor.java

@@ -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;
+    }
 }

fhir-database-utils/src/main/java/com/ibm/fhir/database/utils/query/expression/StringExpNodeVisitor.java

@@ -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).

fhir-database-utils/src/main/java/com/ibm/fhir/database/utils/query/node/ExpNodeVisitor.java

@@ -90,4 +90,11 @@ public interface ExpNodeVisitor<T> {
      * @return
      */
     T sin(T arg);
+
+    /**
+     * SQL RADIANS function
+     * @param arg
+     * @return
+     */
+    T radians(T arg);
 }

fhir-database-utils/src/main/java/com/ibm/fhir/database/utils/query/node/RadiansExpNode.java

@@ -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;
+    }
+}

fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/util/type/NewLocationParmBehaviorUtil.java

@@ -26,6 +26,9 @@
  * variables.
  */
 public class NewLocationParmBehaviorUtil {
+    // the radius of the earth in km (using a spherical approximation)
+    private static int AVERAGE_RADIUS_OF_EARTH = 6371;
+
     /**
      * build location search query based on the bounding areas.
      *
@@ -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
@@ -129,29 +133,32 @@ 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
-            .sin(bind(boundingRadius.getLatitude()))
+        // ACOS(SIN(boundingRadiusLatitude) * SIN(LATITUDE_VALUE) + COS(boundingRadiusLatitude) * COS(LATITUDE_VALUE) * COS(boundingRadiusLongitude - LONGITUDE_VALUE) * R
+
+        double queryLatitudeInRadians = Math.toRadians(boundingRadius.getLatitude());
+        double queryLongitudeInRadians = Math.toRadians(boundingRadius.getLongitude());
+
+        // First, build the fragments for the longitudinal difference (in radians) and the radian of the latitude in the db
+        WhereFragment longitudeDiff = new WhereFragment().bind(queryLongitudeInRadians).sub().radians(col(paramAlias, LONGITUDE_VALUE));
+        WhereFragment radianLatitude = new WhereFragment().radians(col(paramAlias, LATITUDE_VALUE));
+
+        // Then, use those to build the expression that gets passed to ACOS
+        WhereFragment arcRadius = new WhereFragment()
+            .sin(bind(queryLatitudeInRadians))
             .mult()
-            .sin(col(paramAlias, LATITUDE_VALUE))
+            .sin(radianLatitude.getExpression())
             .add()
-            .cos(bind(boundingRadius.getLatitude()))
+            .cos(bind(queryLatitudeInRadians))
             .mult()
-            .cos(col(paramAlias, LATITUDE_VALUE))
+            .cos(radianLatitude.getExpression())
             .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(AVERAGE_RADIUS_OF_EARTH)
+            .lte()
+            .bind(boundingRadius.getRadius())
+            .rightParen();
     }
 }

fhir-persistence-jdbc/src/test/java/com/ibm/fhir/persistence/jdbc/search/test/JDBCSearchNearTest.java

@@ -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;
@@ -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;
@@ -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();
@@ -228,7 +238,8 @@ public void testSearchPositionSearchExactMatchWithinRangeNot() throws Exception
     @Test
     public void testSearchPositionSearchExactMatchWithinRange() throws Exception {
         // 40, -79
-        // Difference to expected location is 1046.6km
+
+        // https://www.movable-type.co.uk/scripts/latlong.html says the expected distance is 466.2 km
         String searchParamCode = "near";
         String queryValue = "40|-79|1046.6|km";
 

fhir-persistence-jdbc/src/test/java/com/ibm/fhir/persistence/jdbc/test/util/LocationParmBehaviorUtilTest.java

@@ -131,26 +131,25 @@ 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);
-        expectedBindVariables.add(10.0);
-        expectedBindVariables.add(20.0);
-        expectedBindVariables.add(20.0);
-        expectedBindVariables.add(10.0);
-        expectedBindVariables.add(10.0);
+        expectedBindVariables.add(Math.toRadians(10.0));
+        expectedBindVariables.add(Math.toRadians(10.0));
+        expectedBindVariables.add(Math.toRadians(20.0));
         expectedBindVariables.add(4.0);
 
         String expectedSql =
-                "(pX.LATITUDE_VALUE <= ? AND pX.LATITUDE_VALUE >= ? AND pX.LONGITUDE_VALUE <= ? AND pX.LONGITUDE_VALUE >= ? AND "
-                + "ACOS(SIN(?) * SIN(pX.LATITUDE_VALUE) + COS(?) * COS(pX.LATITUDE_VALUE) * COS(pX.LONGITUDE_VALUE)) <= ?)";
+                "(ACOS(SIN(?) * SIN(RADIANS(pX.LATITUDE_VALUE)) + COS(?) * COS(RADIANS(pX.LATITUDE_VALUE)) * COS(? - RADIANS(pX.LONGITUDE_VALUE))) * 6371 <= ?)";
 
         BoundingRadius boundingRadius = BoundingRadius.builder().latitude(10.0).longitude(20.0).radius(4.0).build();
         runTest(expectedBindVariables, expectedSql, boundingRadius);
@@ -159,28 +158,27 @@ 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);
 
         List<Object> expectedBindVariables = new ArrayList<>();
-        expectedBindVariables.add(new Double(10.0));
-        expectedBindVariables.add(new Double(10.0));
-        expectedBindVariables.add(new Double(21.0));
-        expectedBindVariables.add(new Double(21.0));
-        expectedBindVariables.add(new Double(10.0));
-        expectedBindVariables.add(new Double(10.0));
+        expectedBindVariables.add(Math.toRadians(10.0));
+        expectedBindVariables.add(Math.toRadians(10.0));
+        expectedBindVariables.add(Math.toRadians(21.0));
         expectedBindVariables.add(new Double(4.0));
         expectedBindVariables.add(-10.0);
         expectedBindVariables.add(11.0);
         expectedBindVariables.add(-20.0);
         expectedBindVariables.add(20.0);
 
         String expectedSql =
-                "((pX.LATITUDE_VALUE <= ? AND pX.LATITUDE_VALUE >= ? AND pX.LONGITUDE_VALUE <= ? AND pX.LONGITUDE_VALUE >= ? AND "
-                + "ACOS(SIN(?) * SIN(pX.LATITUDE_VALUE) + COS(?) * COS(pX.LATITUDE_VALUE) * COS(pX.LONGITUDE_VALUE)) <= ?)"
+                "((ACOS(SIN(?) * SIN(RADIANS(pX.LATITUDE_VALUE)) + COS(?) * COS(RADIANS(pX.LATITUDE_VALUE)) * COS(? - RADIANS(pX.LONGITUDE_VALUE))) * 6371 <= ?)"
                 + " OR "
                 + "(pX.LATITUDE_VALUE >= ? AND pX.LATITUDE_VALUE <= ? AND pX.LONGITUDE_VALUE >= ? AND pX.LONGITUDE_VALUE <= ?))";
 
@@ -189,18 +187,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);