Browse Source

synthetic source: fix scaled_float rounding (#88916)

There were some cases where synthetic source wasn't properly rounding in
round trips. `0.15527719259262085` with a scaling factor of
`2.4206374697469164E16` was round tripping to `0.15527719259262088`
which then round trips up to `0.0.1552771925926209`, rounding the wrong
direction! This fixes the round tripping in this case through ever more
paranoid double checking and nudging.

Closes #88854
Nik Everett 3 years ago
parent
commit
4607182ce8

+ 6 - 0
docs/changelog/88916.yaml

@@ -0,0 +1,6 @@
+pr: 88916
+summary: "Synthetic source: fix `scaled_float` rounding"
+area: "TSDB"
+type: bug
+issues:
+ - 88854

+ 16 - 20
modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/ScaledFloatFieldMapper.java

@@ -704,29 +704,25 @@ public class ScaledFloatFieldMapper extends FieldMapper {
      *   assert scaled2 != Long.MAX_VALUE;
      * }</pre>
      * <p>
-     * We work around this by detecting such cases and artificially bumping them
-     * up by a single digit in the last place, forcing them to always saturate
-     * the {@link Math#round} call.
+     * This can happen sometimes with regular old rounding too, in situations that
+     * aren't entirely clear at the moment. We work around this by detecting when
+     * the round trip wouldn't produce the same encoded value and artificially
+     * bumping them up by a single digit in the last place towards the direction
+     * that would make the round trip consistent. Bumping by a single digit in
+     * the last place is always enough to correct the tiny errors that can sneak
+     * in from the unexpected rounding.
      */
     static double decodeForSyntheticSource(long scaledValue, double scalingFactor) {
-        if (scaledValue == Long.MAX_VALUE) {
-            double max = Long.MAX_VALUE / scalingFactor;
-            if (Math.round(max * scalingFactor) != Long.MAX_VALUE) {
-                double v = max + Math.ulp(max);
-                assert Math.round(v * scalingFactor) == Long.MAX_VALUE;
-                return v;
-            }
-            return max;
-        }
-        if (scaledValue == Long.MIN_VALUE) {
-            double min = Long.MIN_VALUE / scalingFactor;
-            if (Math.round(min * scalingFactor) != Long.MIN_VALUE) {
-                double v = min - Math.ulp(min);
-                assert Math.round(v * scalingFactor) == Long.MIN_VALUE;
-                return v;
+        double v = scaledValue / scalingFactor;
+        long reenc = Math.round(v * scalingFactor);
+        if (reenc != scaledValue) {
+            if (reenc > scaledValue) {
+                v -= Math.ulp(v);
+            } else {
+                v += Math.ulp(v);
             }
-            return min;
+            assert Math.round(v * scalingFactor) == scaledValue : Math.round(v * scalingFactor) + " != " + scaledValue;
         }
-        return scaledValue / scalingFactor;
+        return v;
     }
 }

+ 29 - 13
modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/extras/ScaledFloatFieldMapperTests.java

@@ -383,21 +383,15 @@ public class ScaledFloatFieldMapperTests extends MapperTestCase {
 
             private double round(double d) {
                 long encoded = Math.round(d * scalingFactor);
-                if (encoded == Long.MAX_VALUE) {
-                    double max = Long.MAX_VALUE / scalingFactor;
-                    if (max * scalingFactor != Long.MAX_VALUE) {
-                        return max + Math.ulp(max);
+                double decoded = encoded / scalingFactor;
+                long reencoded = Math.round(decoded * scalingFactor);
+                if (encoded != reencoded) {
+                    if (encoded > reencoded) {
+                        return decoded + Math.ulp(decoded);
                     }
-                    return max;
+                    return decoded - Math.ulp(decoded);
                 }
-                if (encoded == Long.MIN_VALUE) {
-                    double min = Long.MIN_VALUE / scalingFactor;
-                    if (min * scalingFactor != Long.MIN_VALUE) {
-                        return min - Math.ulp(min);
-                    }
-                    return min;
-                }
-                return encoded / scalingFactor;
+                return decoded;
             }
 
             private void mapping(XContentBuilder b) throws IOException {
@@ -487,6 +481,28 @@ public class ScaledFloatFieldMapperTests extends MapperTestCase {
         assertThat(encodeDecode(saturated, scalingFactor), equalTo(max));
     }
 
+    public void testEncodeDecodeRandom() {
+        double scalingFactor = randomDoubleBetween(0, Double.MAX_VALUE, false);
+        double v = randomValue();
+        double once = encodeDecode(v, scalingFactor);
+        double twice = encodeDecode(once, scalingFactor);
+        assertThat(twice, equalTo(once));
+    }
+
+    /**
+     * Tests that a value and scaling factor that won't
+     * properly round trip without a "nudge" to keep
+     * them from rounding in the wrong direction on the
+     * second iteration.
+     */
+    public void testEncodeDecodeNeedNudge() {
+        double scalingFactor = 2.4206374697469164E16;
+        double v = 0.15527719259262085;
+        double once = encodeDecode(v, scalingFactor);
+        double twice = encodeDecode(once, scalingFactor);
+        assertThat(twice, equalTo(once));
+    }
+
     /**
      * Tests that any encoded value with that can that fits in the mantissa of
      * a double precision floating point can be round tripped through synthetic