Repository: gchq/Gaffer (java)

Issue:
ToHllSketch functions do not copy from other HllSketch
**Describe the bug**
`ToHllSketch` and `IterableToHllSketch` can both be intitialised with another HllSketch. It should be copied but is not.
Additionally, these fields should be JsonIgnored as they won't be used over the rest api and wouldn't work anyway.


Pull request:
Gh-3061: Ensure ToHllSketch functions copy
# Related issue

- Resolve #3061

Patch:
diff --git a/library/sketches-library/src/main/java/uk/gov/gchq/gaffer/sketches/CardinalityEntityGenerator.java b/library/sketches-library/src/main/java/uk/gov/gchq/gaffer/sketches/CardinalityEntityGenerator.java
index 7eeaad681e..996b2f4d50 100755
--- a/library/sketches-library/src/main/java/uk/gov/gchq/gaffer/sketches/CardinalityEntityGenerator.java
+++ b/library/sketches-library/src/main/java/uk/gov/gchq/gaffer/sketches/CardinalityEntityGenerator.java
@@ -16,6 +16,7 @@
 
 package uk.gov.gchq.gaffer.sketches;
 
+import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.annotation.JsonTypeInfo;
 
@@ -57,6 +58,7 @@ public abstract class CardinalityEntityGenerator<T> implements OneToManyElementG
      */
     private final Set<String> propertiesToCopy = new HashSet<>();
 
+    @JsonIgnore
     public abstract Function<Object, T> getToSketchFunction();
 
     @Override
diff --git a/library/sketches-library/src/main/java/uk/gov/gchq/gaffer/sketches/datasketches/cardinality/function/IterableToHllSketch.java b/library/sketches-library/src/main/java/uk/gov/gchq/gaffer/sketches/datasketches/cardinality/function/IterableToHllSketch.java
index dd897d0106..846b3ad8c6 100644
--- a/library/sketches-library/src/main/java/uk/gov/gchq/gaffer/sketches/datasketches/cardinality/function/IterableToHllSketch.java
+++ b/library/sketches-library/src/main/java/uk/gov/gchq/gaffer/sketches/datasketches/cardinality/function/IterableToHllSketch.java
@@ -16,6 +16,7 @@
 
 package uk.gov.gchq.gaffer.sketches.datasketches.cardinality.function;
 
+import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonInclude;
 import org.apache.datasketches.hll.HllSketch;
 
@@ -35,24 +36,24 @@ import static uk.gov.gchq.gaffer.sketches.datasketches.cardinality.serialisation
 @Summary("Creates a new HllSketch instance and initialises it from the given iterable")
 public class IterableToHllSketch extends KorypheFunction<Iterable<Object>, HllSketch> {
     private int logK = DEFAULT_LOG_K;
+    @JsonIgnore
     private HllSketch hllSketch;
 
     public IterableToHllSketch() {
+        hllSketch = new HllSketch(logK);
     }
 
     public IterableToHllSketch(final int logK) {
         this.logK = logK;
+        hllSketch = new HllSketch(logK);
     }
 
     public IterableToHllSketch(final HllSketch hllSketch) {
-        this.hllSketch = hllSketch;
+        setHllSketch(hllSketch);
     }
 
     @Override
     public HllSketch apply(final Iterable<Object> iterable) {
-        if (hllSketch == null) {
-            hllSketch = new HllSketch(logK);
-        }
         if (nonNull(iterable)) {
             for (final Object o : iterable) {
                 if (nonNull(o)) {
@@ -94,6 +95,10 @@ public class IterableToHllSketch extends KorypheFunction<Iterable<Object>, HllSk
     }
 
     public void setHllSketch(final HllSketch hllSketch) {
-        this.hllSketch = hllSketch;
+        if (hllSketch != null) {
+            this.hllSketch = hllSketch.copy();
+        } else {
+            this.hllSketch = new HllSketch(logK);
+        }
     }
 }
diff --git a/library/sketches-library/src/main/java/uk/gov/gchq/gaffer/sketches/datasketches/cardinality/function/ToHllSketch.java b/library/sketches-library/src/main/java/uk/gov/gchq/gaffer/sketches/datasketches/cardinality/function/ToHllSketch.java
index 9b5626985d..41c830f281 100644
--- a/library/sketches-library/src/main/java/uk/gov/gchq/gaffer/sketches/datasketches/cardinality/function/ToHllSketch.java
+++ b/library/sketches-library/src/main/java/uk/gov/gchq/gaffer/sketches/datasketches/cardinality/function/ToHllSketch.java
@@ -16,6 +16,7 @@
 
 package uk.gov.gchq.gaffer.sketches.datasketches.cardinality.function;
 
+import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonInclude;
 import org.apache.datasketches.hll.HllSketch;
 
@@ -35,24 +36,24 @@ import static uk.gov.gchq.gaffer.sketches.datasketches.cardinality.serialisation
 @JsonInclude(JsonInclude.Include.NON_DEFAULT)
 public class ToHllSketch extends KorypheFunction<Object, HllSketch> {
     private int logK = DEFAULT_LOG_K;
+    @JsonIgnore
     private HllSketch hllSketch;
 
     public ToHllSketch() {
+        hllSketch = new HllSketch(logK);
     }
 
     public ToHllSketch(final int logK) {
         this.logK = logK;
+        hllSketch = new HllSketch(logK);
     }
 
     public ToHllSketch(final HllSketch hllSketch) {
-        this.hllSketch = hllSketch;
+        setHllSketch(hllSketch);
     }
 
     @Override
     public HllSketch apply(final Object o) {
-        if (hllSketch == null) {
-            hllSketch = new HllSketch(logK);
-        }
         if (nonNull(o)) {
             if (o instanceof String) {
                 hllSketch.update((String) o);
@@ -90,6 +91,10 @@ public class ToHllSketch extends KorypheFunction<Object, HllSketch> {
     }
 
     public void setHllSketch(final HllSketch hllSketch) {
-        this.hllSketch = hllSketch;
+        if (hllSketch != null) {
+            this.hllSketch = hllSketch.copy();
+        } else {
+            this.hllSketch = new HllSketch(logK);
+        }
     }
 }


Test patch:
diff --git a/library/sketches-library/src/test/java/uk/gov/gchq/gaffer/sketches/datasketches/cardinality/function/IterableToHllSketchTest.java b/library/sketches-library/src/test/java/uk/gov/gchq/gaffer/sketches/datasketches/cardinality/function/IterableToHllSketchTest.java
index 95ea547248..70e2199b6c 100644
--- a/library/sketches-library/src/test/java/uk/gov/gchq/gaffer/sketches/datasketches/cardinality/function/IterableToHllSketchTest.java
+++ b/library/sketches-library/src/test/java/uk/gov/gchq/gaffer/sketches/datasketches/cardinality/function/IterableToHllSketchTest.java
@@ -46,6 +46,18 @@ class IterableToHllSketchTest extends FunctionTest<IterableToHllSketch> {
         assertThat(result.getEstimate()).isEqualTo(0);
     }
 
+    @Test
+    public void shouldCreateDefaultSketchWhenNullConstructor() {
+        // Given
+        final IterableToHllSketch iterableToHllSketch = new IterableToHllSketch(null);
+
+        // When
+        final int logK = iterableToHllSketch.getLogK();
+
+        // Then
+        assertThat(logK).isEqualTo(DEFAULT_LOG_K);
+    }
+
     @Test
     public void shouldCreateHllSketch() {
         // Given
@@ -102,7 +114,7 @@ class IterableToHllSketchTest extends FunctionTest<IterableToHllSketch> {
     public void shouldCorrectlyCreateFromAnotherHllSketch() {
         // Given
         final HllSketch anotherSketch = new HllSketch(5);
-        IterableToHllSketch iterableToHllSketch = new IterableToHllSketch(anotherSketch);
+        final IterableToHllSketch iterableToHllSketch = new IterableToHllSketch(anotherSketch);
         final List<Object> input = Arrays.asList("one", "one", "two", "two", "three");
 
         // When
@@ -112,6 +124,24 @@ class IterableToHllSketchTest extends FunctionTest<IterableToHllSketch> {
         assertThat(result.getLgConfigK()).isEqualTo(5);
     }
 
+    @Test
+    public void shouldCorrectlyCopyAnotherHllSketch() {
+        // Given
+        final HllSketch anotherSketch = new HllSketch();
+        anotherSketch.update("second");
+        anotherSketch.update("third");
+
+        final IterableToHllSketch iterableToHllSketch = new IterableToHllSketch(anotherSketch);
+        final List<Object> input = Arrays.asList("one", "one", "two", "two", "three");
+
+        // When
+        final HllSketch result = iterableToHllSketch.apply(input);
+
+        // Then
+        assertThat(result.getEstimate()).isCloseTo(5, Percentage.withPercentage(0.001));
+        assertThat(anotherSketch.getEstimate()).isCloseTo(2, Percentage.withPercentage(0.001));
+    }
+
     @Override
     protected Class[] getExpectedSignatureInputClasses() {
         return new Class[]{Iterable.class};
diff --git a/library/sketches-library/src/test/java/uk/gov/gchq/gaffer/sketches/datasketches/cardinality/function/ToHllSketchTest.java b/library/sketches-library/src/test/java/uk/gov/gchq/gaffer/sketches/datasketches/cardinality/function/ToHllSketchTest.java
index 6e81ad457c..2bb7e399af 100644
--- a/library/sketches-library/src/test/java/uk/gov/gchq/gaffer/sketches/datasketches/cardinality/function/ToHllSketchTest.java
+++ b/library/sketches-library/src/test/java/uk/gov/gchq/gaffer/sketches/datasketches/cardinality/function/ToHllSketchTest.java
@@ -34,22 +34,34 @@ class ToHllSketchTest extends FunctionTest<ToHllSketch> {
     @Test
     public void shouldCreateEmptyWhenNull() {
         // Given
-        ToHllSketch toHllSketch = new ToHllSketch();
+        final ToHllSketch toHllSketch = new ToHllSketch();
 
         // When
-        HllSketch result = toHllSketch.apply(null);
+        final HllSketch result = toHllSketch.apply(null);
 
         // Then
         assertThat(result.getEstimate()).isEqualTo(0);
     }
 
+    @Test
+    public void shouldCreateDefaultSketchWhenNullConstructor() {
+        // Given
+        final ToHllSketch toHllSketch = new ToHllSketch(null);
+
+        // When
+        final int logK = toHllSketch.getLogK();
+
+        // Then
+        assertThat(logK).isEqualTo(DEFAULT_LOG_K);
+    }
+
     @Test
     public void shouldCreateHllSketch() {
         // Given
-        ToHllSketch toHllSketch = new ToHllSketch();
+        final ToHllSketch toHllSketch = new ToHllSketch();
 
         // When
-        HllSketch result = toHllSketch.apply("input");
+        final HllSketch result = toHllSketch.apply("input");
 
         // Then
         assertThat(result.getEstimate()).isEqualTo(1);
@@ -97,7 +109,7 @@ class ToHllSketchTest extends FunctionTest<ToHllSketch> {
     public void shouldCorrectlyCreateFromAnotherHllSketch() {
         // Given
         final HllSketch anotherSketch = new HllSketch(5);
-        ToHllSketch toHllSketch = new ToHllSketch(anotherSketch);
+        final ToHllSketch toHllSketch = new ToHllSketch(anotherSketch);
 
         // When
         final HllSketch result = toHllSketch.apply("input");
@@ -106,6 +118,23 @@ class ToHllSketchTest extends FunctionTest<ToHllSketch> {
         assertThat(result.getLgConfigK()).isEqualTo(5);
     }
 
+    @Test
+    public void shouldCorrectlyCopyAnotherHllSketch() {
+        // Given
+        final HllSketch anotherSketch = new HllSketch();
+        anotherSketch.update("second");
+        anotherSketch.update("third");
+
+        final ToHllSketch toHllSketch = new ToHllSketch(anotherSketch);
+
+        // When
+        final HllSketch result = toHllSketch.apply("input");
+
+        // Then
+        assertThat(result.getEstimate()).isCloseTo(3, Percentage.withPercentage(0.001));
+        assertThat(anotherSketch.getEstimate()).isCloseTo(2, Percentage.withPercentage(0.001));
+    }
+
     @Override
     protected Class[] getExpectedSignatureInputClasses() {
         return new Class[]{Object.class};
@@ -124,7 +153,7 @@ class ToHllSketchTest extends FunctionTest<ToHllSketch> {
                 new ToHllSketch();
         // When
         final String json = new String(JSONSerialiser.serialise(toHllSketch));
-        ToHllSketch deserialisedToHllSketch = JSONSerialiser.deserialise(json, ToHllSketch.class);
+        final ToHllSketch deserialisedToHllSketch = JSONSerialiser.deserialise(json, ToHllSketch.class);
         // Then
         assertEquals(toHllSketch, deserialisedToHllSketch);
         assertEquals("{\"class\":\"uk.gov.gchq.gaffer.sketches.datasketches.cardinality.function.ToHllSketch\"}", json);