Repository: ipums/hlink (language: python)

Issue:
`JaroWinklerSimilarity` returns 1 for two empty strings
When we updated Scala Apache Commons Text from 1.4 to 1.9 in version 3.2.0 of hlink, we started using `JaroWinklerSimilarity` instead of `JaroWinklerDistance`. Commons Text renamed `JaroWinklerDistance` to `JaroWinklerSimilarity` because it was actually computing the similarity between two strings. But the behavior on empty strings has also changed, and we didn't notice that until now.

With the new `JaroWinklerSimilarity`, two empty strings `""` and `""` have computed similarity 1.0. This used to be 0.0 with the old `JaroWinklerDistance`. We may need to have a special case to handle this in hlink, as we would like two empty strings to have similarity 0.0.
`JaroWinklerSimilarity` returns 1 for two empty strings
When we updated Scala Apache Commons Text from 1.4 to 1.9 in version 3.2.0 of hlink, we started using `JaroWinklerSimilarity` instead of `JaroWinklerDistance`. Commons Text renamed `JaroWinklerDistance` to `JaroWinklerSimilarity` because it was actually computing the similarity between two strings. But the behavior on empty strings has also changed, and we didn't notice that until now.

With the new `JaroWinklerSimilarity`, two empty strings `""` and `""` have computed similarity 1.0. This used to be 0.0 with the old `JaroWinklerDistance`. We may need to have a special case to handle this in hlink, as we would like two empty strings to have similarity 0.0.


Discussion / hints:
I believe all of our Jaro Winkler calls go through [SerJaroWinklerSimilarity](https://github.com/ipums/hlink/blob/465d7ce7882f680a60e5c569d01fd9bf02e8b2f2/scala_jar/src/main/scala/com/isrdi/udfs/SerJaroWinklerSimilarity.scala) . The only method called on SerJaroWinkler is `apply`. We should override the `apply` method to first check if either string is 0 and return 0.0 if true, else call `super.apply()` (or however you do it in Java).
I think this should do it.

```scala
class SerJaroWinklerSimilarity extends JaroWinklerSimilarity with Serializable {
  override def apply(left: CharSequence, right: CharSequence): Double = {
    if (left == "" && right == "") {
      0.0
    } else {
      super.apply(left, right)
    }
  }
}
```
I believe all of our Jaro Winkler calls go through [SerJaroWinklerSimilarity](https://github.com/ipums/hlink/blob/465d7ce7882f680a60e5c569d01fd9bf02e8b2f2/scala_jar/src/main/scala/com/isrdi/udfs/SerJaroWinklerSimilarity.scala) . The only method called on SerJaroWinkler is `apply`. We should override the `apply` method to first check if either string is 0 and return 0.0 if true, else call `super.apply()` (or however you do it in Java).
I think this should do it.

```scala
class SerJaroWinklerSimilarity extends JaroWinklerSimilarity with Serializable {
  override def apply(left: CharSequence, right: CharSequence): Double = {
    if (left == "" && right == "") {
      0.0
    } else {
      super.apply(left, right)
    }
  }
}
```

Fix patch:
diff --git a/hlink/spark/jars/hlink_lib-assembly-1.0.jar b/hlink/spark/jars/hlink_lib-assembly-1.0.jar
index f987907..5af9ca5 100644
Binary files a/hlink/spark/jars/hlink_lib-assembly-1.0.jar and b/hlink/spark/jars/hlink_lib-assembly-1.0.jar differ
diff --git a/scala_jar/src/main/scala/com/isrdi/udfs/SerJaroWinklerSimilarity.scala b/scala_jar/src/main/scala/com/isrdi/udfs/SerJaroWinklerSimilarity.scala
index 8fb2769..a8208aa 100644
--- a/scala_jar/src/main/scala/com/isrdi/udfs/SerJaroWinklerSimilarity.scala
+++ b/scala_jar/src/main/scala/com/isrdi/udfs/SerJaroWinklerSimilarity.scala
@@ -6,4 +6,23 @@
 package com.isrdi.udfs
 import org.apache.commons.text.similarity._
 
-class SerJaroWinklerSimilarity extends JaroWinklerSimilarity with Serializable { }
+class SerJaroWinklerSimilarity extends JaroWinklerSimilarity with Serializable {
+  // If either input string is empty, immediately return 0. Otherwise, return
+  // the value of calling JaroWinklerSimilarity's apply function.
+  // JaroWinklerSimilarity.apply("", "") returns 1, but for our use case it makes
+  // more sense for two empty strings to have similarity 0.
+  //
+  // By my understanding, the comparison of two empty strings is essentially
+  // undefined. To me it makes sense to directly apply the definition of
+  // Jaro-Winkler similarity to two empty strings and get a similarity of 0.
+  // But this has the unfortunate side effect of making the distance between two
+  // empty strings 1 - 0 = 1. So I understand the decision to make the
+  // similarity 1 and the distance 0 as well.
+  override def apply(left: CharSequence, right: CharSequence): java.lang.Double = {
+    if (left == "" || right == "") {
+      0.0
+    } else {
+      super.apply(left, right)
+    }
+  }
+}


Test patch:
diff --git a/hlink/tests/scala_udf_test.py b/hlink/tests/scala_udf_test.py
new file mode 100644
index 0000000..1374cd9
--- /dev/null
+++ b/hlink/tests/scala_udf_test.py
@@ -0,0 +1,29 @@
+# This file is part of the ISRDI's hlink.
+# For copyright and licensing information, see the NOTICE and LICENSE files
+# in this project's top-level directory, and also on-line at:
+#   https://github.com/ipums/hlink
+
+import pytest
+
+
+def test_jw_empty_strings(spark):
+    [row] = spark.sql("SELECT jw('', '') AS jw_result").collect()
+    assert row.jw_result < 0.0001
+
+
+@pytest.mark.parametrize("left,right", [("z", "z"), ("ambulate", "ambulate")])
+def test_jw_matching_strings(spark, left: str, right: str):
+    [row] = spark.sql(f"SELECT jw('{left}', '{right}') AS jw_result").collect()
+    assert row.jw_result > 0.9999
+
+
+@pytest.mark.parametrize("left,right", [("zzzzzz", "sleepy"), ("no", "shared letters")])
+def test_jw_completely_different_strings(spark, left: str, right: str):
+    [row] = spark.sql(f"SELECT jw('{left}', '{right}') AS jw_result").collect()
+    assert row.jw_result < 0.0001
+
+
+@pytest.mark.parametrize("left,right", [("", "discombobulated"), ("GlyphidMenace", "")])
+def test_jw_one_empty_string(spark, left: str, right: str):
+    [row] = spark.sql(f"SELECT jw('{left}', '{right}') AS jw_result").collect()
+    assert row.jw_result < 0.0001