feat!: fractional bucketing improvements

tools/flagd-core/src/main/java/dev/openfeature/contrib/tools/flagd/core/targeting/Fractional.java

@@ -3,9 +3,13 @@
 import io.github.jamsesso.jsonlogic.JsonLogicException;
 import io.github.jamsesso.jsonlogic.evaluator.JsonLogicEvaluationException;
 import io.github.jamsesso.jsonlogic.evaluator.expressions.PreEvaluatedArgumentsExpression;
+import java.math.BigDecimal;
+import java.math.BigInteger;
+import java.nio.ByteBuffer;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.BitSet;
 import java.util.List;
 import lombok.Getter;
 import lombok.extern.slf4j.Slf4j;
@@ -33,13 +37,19 @@ public Object evaluate(List arguments, Object data, String jsonPath) throws Json
         // check optional string target in first arg
         Object arg1 = arguments.get(0);
 
-        final String bucketBy;
+        final byte[] bucketBy;
         final Object[] distributions;
 
         if (arg1 instanceof String) {
-            // first arg is a String, use for bucketing
-            bucketBy = (String) arg1;
-
+            bucketBy = ((String) arg1).getBytes(StandardCharsets.UTF_8);
+            Object[] source = arguments.toArray();
+            distributions = Arrays.copyOfRange(source, 1, source.length);
+        } else if (arg1 instanceof Number) {
+            bucketBy = numberToByteArray((Number) arg1);
+            Object[] source = arguments.toArray();
+            distributions = Arrays.copyOfRange(source, 1, source.length);
+        } else if (arg1 instanceof Boolean) {
+            bucketBy = new byte[] {(byte) (((boolean) arg1) ? 1 : 0)};
             Object[] source = arguments.toArray();
             distributions = Arrays.copyOfRange(source, 1, source.length);
         } else {
@@ -49,7 +59,7 @@ public Object evaluate(List arguments, Object data, String jsonPath) throws Json
                 return null;
             }
 
-            bucketBy = properties.getFlagKey() + properties.getTargetingKey();
+            bucketBy = (properties.getFlagKey() + properties.getTargetingKey()).getBytes(StandardCharsets.UTF_8);
             distributions = arguments.toArray();
         }
 
@@ -71,29 +81,80 @@ public Object evaluate(List arguments, Object data, String jsonPath) throws Json
         return distributeValue(bucketBy, propertyList, totalWeight, jsonPath);
     }
 
+    private byte[] numberToByteArray(Number number) {
+        if (number instanceof Integer) {
+            return new byte[] {
+                    (byte) ((int) number >> 24),
+                    (byte) ((int) number >> 16),
+                    (byte) ((int) number >> 8),
+                    (byte) ((int) number)
+            };
+        } else if (number instanceof Double) {
+            return numberToByteArray(Double.doubleToLongBits((Double) number));
+        } else if (number instanceof Long) {
+            return new byte[] {
+                    (byte) ((long) number >> 56),
+                    (byte) ((long) number >> 48),
+                    (byte) ((long) number >> 40),
+                    (byte) ((long) number >> 32),
+                    (byte) ((long) number >> 24),
+                    (byte) ((long) number >> 16),
+                    (byte) ((long) number >> 8),
+                    (byte) ((long) number)
+            };
+        } else if (number instanceof BigInteger) {
+            return ((BigInteger) number).toByteArray();
+        } else if (number instanceof Byte) {
+            return new byte[] {(byte) number};
+        } else if (number instanceof Short) {
+            return new byte[] {
+                    (byte) ((short) number >> 8),
+                    (byte) ((short) number)
+            };
+        } else if (number instanceof Float) {
+            return numberToByteArray(Float.floatToIntBits((Float) number));
+        } else if (number instanceof BigDecimal) {
+            return numberToByteArray(Double.doubleToLongBits(number.doubleValue()));
+        } else {
+            throw new IllegalArgumentException("Unsupported number type: " + number.getClass());
+        }
+    }
+
     private static String distributeValue(
-            final String hashKey, final List<FractionProperty> propertyList, int totalWeight, String jsonPath)
+            final byte[] hashKey, final List<FractionProperty> propertyList, final int totalWeight,
+            final String jsonPath)
+            throws JsonLogicEvaluationException {
+        int mmrHash = MurmurHash3.hash32x86(hashKey, 0, hashKey.length, 0);
+        return distributeValueFromHash(mmrHash, propertyList, totalWeight, jsonPath);
+    }
+
+    static String distributeValueFromHash(
+            final int hash, final List<FractionProperty> propertyList, final int totalWeight,
+            final String jsonPath)
             throws JsonLogicEvaluationException {
-        byte[] bytes = hashKey.getBytes(StandardCharsets.UTF_8);
-        int mmrHash = MurmurHash3.hash32x86(bytes, 0, bytes.length, 0);
-        float bucket = Math.abs(mmrHash) * 1.0f / Integer.MAX_VALUE * 100;
+        long longHash = Math.abs((long) hash);
+        if (hash < 0) {
+            // preserve the MSB (sign) of the hash, which would get lost in a typecast and in Math.abs
+            longHash = longHash | (1L << 31);
+        }
+        int bucket = Math.abs((int) ((longHash * totalWeight) >> 32));
 
-        float bucketSum = 0;
+        int bucketSum = 0;
         for (FractionProperty p : propertyList) {
-            bucketSum += p.getPercentage(totalWeight);
+            bucketSum += p.weight;
 
-            if (bucket < bucketSum) {
+            if (bucket <= bucketSum) {
                 return p.getVariant();
             }
         }
 
         // this shall not be reached
-        throw new JsonLogicEvaluationException("Unable to find a correct bucket", jsonPath);
+        throw new JsonLogicEvaluationException("Unable to find a correct bucket for hash " + hash, jsonPath);
     }
 
     @Getter
     @SuppressWarnings({"checkstyle:NoFinalizer"})
-    private static class FractionProperty {
+    static class FractionProperty {
         private final String variant;
         private final int weight;
 
@@ -122,19 +183,13 @@ protected final void finalize() {
             if (array.size() >= 2) {
                 // second element must be a number
                 if (!(array.get(1) instanceof Number)) {
-                    throw new JsonLogicException("Second element of the fraction property is not a number", jsonPath);
+                    throw new JsonLogicException("Second element of the fraction property is not a number",
+                            jsonPath);
                 }
                 weight = ((Number) array.get(1)).intValue();
             } else {
                 weight = 1;
             }
         }
-
-        float getPercentage(int totalWeight) {
-            if (weight == 0) {
-                return 0;
-            }
-            return (float) (weight * 100) / totalWeight;
-        }
     }
 }

tools/flagd-core/src/test/java/dev/openfeature/contrib/tools/flagd/core/targeting/FractionalTest.java

@@ -1,27 +1,42 @@
 package dev.openfeature.contrib.tools.flagd.core.targeting;
 
 import static dev.openfeature.contrib.tools.flagd.core.targeting.Operator.*;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Named.named;
 import static org.junit.jupiter.params.provider.Arguments.arguments;
 
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.databind.ObjectMapper;
+import io.cucumber.java.sl.In;
+import io.github.jamsesso.jsonlogic.JsonLogicException;
 import io.github.jamsesso.jsonlogic.evaluator.JsonLogicEvaluationException;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtensionContext;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.converter.ArgumentConversionException;
 import org.junit.jupiter.params.converter.ConvertWith;
 import org.junit.jupiter.params.converter.TypedArgumentConverter;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.ArgumentsProvider;
+import org.junit.jupiter.params.provider.ArgumentsSource;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.junit.jupiter.params.support.ParameterDeclarations;
 
 class FractionalTest {
 
@@ -47,6 +62,67 @@ void validate_emptyJson_targetingReturned(@ConvertWith(FileContentConverter.clas
         assertEquals(testData.result, evaluate);
     }
 
+    @ParameterizedTest
+    @ValueSource(ints = {
+            0,
+            1,
+            -1,
+            Integer.MAX_VALUE,
+            Integer.MAX_VALUE - 1,
+            Integer.MIN_VALUE,
+            Integer.MIN_VALUE + 1
+    })
+    void edgeCasesDoNotThrow(int hash) throws JsonLogicException {
+        int totalWeight = 8;
+        int buckets = 4;
+        List<Fractional.FractionProperty> bucketsList = new ArrayList<>(buckets);
+        for (int i = 0; i < buckets; i++) {
+            bucketsList.add(new Fractional.FractionProperty(List.of("bucket" + i, totalWeight / buckets), ""));
+        }
+
+        AtomicReference<String> result = new AtomicReference<>();
+        assertDoesNotThrow(() -> result.set(Fractional.distributeValueFromHash(hash, bucketsList, totalWeight, "")));
+
+        assertNotNull(result.get());
+        assertTrue(result.get().startsWith("bucket"));
+    }
+
+    @Test
+    void statistics() throws JsonLogicException {
+        int totalWeight = Integer.MAX_VALUE;
+        int buckets = 16;
+        int[] hits = new int[buckets];
+        List<Fractional.FractionProperty> bucketsList = new ArrayList<>(buckets);
+        int weight = totalWeight / buckets;
+        for (int i = 0; i < buckets - 1; i++) {
+            bucketsList.add(new Fractional.FractionProperty(List.of("" + i, weight), ""));
+        }
+        bucketsList.add(
+                new Fractional.FractionProperty(List.of("" + (buckets - 1), totalWeight - weight * (buckets - 1)), "")
+        );
+
+        for (long i = Integer.MIN_VALUE; i <= Integer.MAX_VALUE; i += 127) {
+            String bucketStr = Fractional.distributeValueFromHash((int) i, bucketsList, totalWeight, "");
+            int bucket = Integer.parseInt(bucketStr);
+            hits[bucket]++;
+        }
+
+        int min = Integer.MAX_VALUE;
+        int max = Integer.MIN_VALUE;
+        for (int i = 0; i < hits.length; i++) {
+            int current = hits[i];
+            if (current < min) {
+                min = current;
+            }
+            if (current > max) {
+                max = current;
+            }
+        }
+
+        int delta = max - min;
+        assertTrue(delta < 3, "Delta should be less than 5, but was " + delta);
+    }
+
     public static Stream<?> allFilesInDir() throws IOException {
         return Files.list(Paths.get("src", "test", "resources", "fractional"))
                 .map(path -> arguments(named(path.getFileName().toString(), path)));

tools/flagd-core/src/test/java/dev/openfeature/contrib/tools/flagd/core/targeting/OperatorTest.java

@@ -75,7 +75,7 @@ void testFlagPropertiesConstructor() {
     }
 
     @Test
-    void fractionalTestA() throws TargetingRuleException {
+    void fractionalTestB() throws TargetingRuleException {
         // given
 
         // fractional rule with email as expression key
@@ -112,11 +112,11 @@ void fractionalTestA() throws TargetingRuleException {
         Object evalVariant = OPERATOR.apply("headerColor", targetingRule, new ImmutableContext(ctxData));
 
         // then
-        assertEquals("yellow", evalVariant);
+        assertEquals("blue", evalVariant);
     }
 
     @Test
-    void fractionalTestB() throws TargetingRuleException {
+    void fractionalTestA() throws TargetingRuleException {
         // given
 
         // fractional rule with email as expression key
@@ -153,7 +153,7 @@ void fractionalTestB() throws TargetingRuleException {
         Object evalVariant = OPERATOR.apply("headerColor", targetingRule, new ImmutableContext(ctxData));
 
         // then
-        assertEquals("blue", evalVariant);
+        assertEquals("red", evalVariant);
     }
 
     @Test

tools/flagd-core/src/test/resources/fractional/boolean.json

@@ -0,0 +1,14 @@
+{
+  "rule": [
+    true,
+    [
+      "blue",
+      50
+    ],
+    [
+      "green",
+      70
+    ]
+  ],
+  "result": "blue"
+}

tools/flagd-core/src/test/resources/fractional/largeDouble.json

@@ -0,0 +1,14 @@
+{
+  "rule": [
+    9999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999.9,
+    [
+      "blue",
+      50
+    ],
+    [
+      "green",
+      70
+    ]
+  ],
+  "result": "blue"
+}

tools/flagd-core/src/test/resources/fractional/largeInt.json

@@ -0,0 +1,14 @@
+{
+  "rule": [
+    9999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999999,
+    [
+      "blue",
+      50
+    ],
+    [
+      "green",
+      70
+    ]
+  ],
+  "result": "blue"
+}

tools/flagd-core/src/test/resources/fractional/selfContainedFractionalB.json

@@ -10,5 +10,5 @@
       50
     ]
   ],
-  "result": "blue"
+  "result": "red"
 }

tools/flagd-core/src/test/resources/fractional/string.json

@@ -0,0 +1,14 @@
+{
+  "rule": [
+    "some string",
+    [
+      "blue",
+      50
+    ],
+    [
+      "green",
+      70
+    ]
+  ],
+  "result": "blue"
+}