From f6fc702d191ce96931e1cacc65588a454f74fe6e Mon Sep 17 00:00:00 2001 From: ffccites <99155080+PDGGK@users.noreply.github.com> Date: Thu, 13 Aug 2026 22:11:07 +1000 Subject: [PATCH 1/2] Accept integral JSON values that a double represents exactly doubleValueExtractor's validator compared asLong() against a round-trip through asInt(): && jsonNode.asLong() == (long) (double) jsonNode.asInt() asInt() truncates anything outside int range, so the two sides can never agree for a larger integral literal and the value is rejected -- even when a double holds it exactly. Epoch millis is the everyday case: {"f": 1609459200000} against a DOUBLE field fails today. Swapping asInt() for asLong() looks like the fix and is not. (double) Long.MAX_VALUE rounds up to 2^63, and narrowing 2^63 back to long saturates at Long.MAX_VALUE rather than overflowing, so the round-trip appears to succeed and the extractor stores 9223372036854775808 -- an over-rejection traded for silent corruption. Comparing through BigDecimal is exact, and it is what the decimal branch directly below already does. Tests: four supported cases (epoch millis, its negative, 2^31, 2^53) and two rejections (2^53+1 and Long.MAX_VALUE). The rejections get a method each because testUnsupportedConversion uses the ExpectedException rule, which is satisfied by the first exception to leave the test method -- a second call in the same body never runs. master fails the supported case; the asLong() variant fails the Long.MAX_VALUE case; this passes 78/78. --- .../beam/sdk/util/RowJsonValueExtractors.java | 11 ++++++-- .../org/apache/beam/sdk/util/RowJsonTest.java | 27 +++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowJsonValueExtractors.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowJsonValueExtractors.java index 2179b20010dc..b09dbf8e7135 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowJsonValueExtractors.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowJsonValueExtractors.java @@ -132,10 +132,17 @@ static ValueExtractor doubleValueExtractor() { jsonNode -> jsonNode.isDouble() - // Either a long number which allows lossless conversion to float + // Either an integral number which allows lossless conversion to double. + // Compared through BigDecimal, the same way the decimal branch below is: a + // long round-trip cannot express this, because narrowing an out-of-range + // double back to long saturates rather than overflowing, so Long.MAX_VALUE + // would look like it survived the trip when it did not. || (jsonNode.isIntegralNumber() && jsonNode.canConvertToLong() - && jsonNode.asLong() == (long) (double) jsonNode.asInt()) + && jsonNode + .decimalValue() + .compareTo(BigDecimal.valueOf(jsonNode.doubleValue())) + == 0) // Or a decimal number which allows lossless conversion to float || (jsonNode.isFloatingPointNumber() diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/util/RowJsonTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/util/RowJsonTest.java index 81f69b62c53b..bd965e85c43e 100644 --- a/sdks/java/core/src/test/java/org/apache/beam/sdk/util/RowJsonTest.java +++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/util/RowJsonTest.java @@ -575,6 +575,14 @@ public void testSupportedDoubleConversions() throws Exception { testSupportedConversion(FieldType.DOUBLE, DOUBLE_STRING, DOUBLE_VALUE); testSupportedConversion(FieldType.DOUBLE, FLOAT_STRING, (double) FLOAT_VALUE); testSupportedConversion(FieldType.DOUBLE, INT_STRING, (double) INT_VALUE); + + // Integral literals beyond int range that a double still represents exactly. Epoch millis + // is the everyday case; 2^31 is the exact boundary where the old asInt() truncation kicked + // in, and 2^53 is the largest contiguous integral double. + testSupportedConversion(FieldType.DOUBLE, "1609459200000", 1609459200000.0d); + testSupportedConversion(FieldType.DOUBLE, "-1609459200000", -1609459200000.0d); + testSupportedConversion(FieldType.DOUBLE, "2147483648", 2147483648.0d); + testSupportedConversion(FieldType.DOUBLE, "9007199254740992", 9007199254740992.0d); } @Test @@ -678,6 +686,25 @@ public void testUnsupportedDoubleConversions() throws Exception { testUnsupportedConversion(FieldType.DOUBLE, LONG_STRING); // too large to fit } + // The two cases below get a method each on purpose. testUnsupportedConversion relies on the + // ExpectedException rule, which is satisfied by the first exception to leave the test method, + // so a second call in the same body never runs and the assertion would be silently dead. + + @Test + public void testUnsupportedDoubleConversionJustPastContiguousRange() throws Exception { + // 2^53 + 1, the first integer a double cannot represent. + testUnsupportedConversion(FieldType.DOUBLE, "9007199254740993"); + } + + @Test + public void testUnsupportedDoubleConversionAtLongMaxValue() throws Exception { + // Long.MAX_VALUE, and the guard against "fixing" this extractor by swapping asInt() for + // asLong(). (double) Long.MAX_VALUE rounds up to 2^63, and narrowing 2^63 back to long + // saturates at Long.MAX_VALUE, so a long round-trip wrongly accepts it and stores + // 9223372036854775808. LONG_STRING above is 2^63 - 2 and does not exercise this. + testUnsupportedConversion(FieldType.DOUBLE, "9223372036854775807"); + } + private void testUnsupportedConversion(FieldType fieldType, String jsonFieldValue) throws Exception { From 8bdec7b044604d2c3a949166fb68c99185f575d6 Mon Sep 17 00:00:00 2001 From: Zihan Dai <99155080+PDGGK@users.noreply.github.com> Date: Sat, 15 Aug 2026 04:07:57 +1000 Subject: [PATCH 2/2] Reject an integral JSON value that a float only saturates back to floatValueExtractor validates an integral literal by round-tripping it through float and back to int: jsonNode.asInt() == (int) (float) jsonNode.asInt() Narrowing a float that is out of int range saturates rather than overflowing, so the trip is not the identity check it looks like. Measured across the interesting values: asInt (float)asInt (int)(float) current correct 2147483647 2.14748365E9 2147483647 accept REJECT -2147483648 -2.14748365E9 -2147483648 accept accept 16777216 1.6777216E7 16777216 accept accept 16777217 1.6777216E7 16777216 reject reject 2147483583 2.14748352E9 2147483520 reject reject Integer.MAX_VALUE is the one value the check gets wrong: the float it goes through is 2147483648, and narrowing that back to int saturates at 2147483647, so the equality holds and the value is accepted. The extractor then stores 2147483648.0 -- a document saying 2147483647 reads back as a different number, with no error. Compared through BigDecimal instead, matching the doubleValueExtractor branch this PR already fixes. Integer.MIN_VALUE and 2^24 stay accepted: both are exactly representable, so this is not an across-the-board tightening. Two supported cases go into the existing method. The unsupported case gets its own, for the reason noted there: the ExpectedException rule is satisfied by the first exception to leave the method, so a later call in the same body never runs. That is also why the existing testUnsupportedFloatConversions cannot cover this -- its INT_STRING call is the fourth in the body and has never executed. Reverting only the float validator fails testUnsupportedFloatConversionAtIntegerMaxValue and nothing else: 79 tests, 1 failed. --- .../beam/sdk/util/RowJsonValueExtractors.java | 11 +++++++++-- .../org/apache/beam/sdk/util/RowJsonTest.java | 19 ++++++++++++++++++- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowJsonValueExtractors.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowJsonValueExtractors.java index b09dbf8e7135..e769212fce76 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowJsonValueExtractors.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowJsonValueExtractors.java @@ -113,10 +113,17 @@ static ValueExtractor floatValueExtractor() { || (jsonNode.isFloatingPointNumber() && jsonNode.doubleValue() == (double) (float) jsonNode.doubleValue()) - // Or an integer number which allows lossless conversion to float + // Or an integral number which allows lossless conversion to float. + // Compared through BigDecimal for the same reason as the double branch + // below: narrowing an out-of-range float back to int saturates rather than + // overflowing, so Integer.MAX_VALUE survives an int round-trip even though + // the float it went through is 2147483648. || (jsonNode.isIntegralNumber() && jsonNode.canConvertToInt() - && jsonNode.asInt() == (int) (float) jsonNode.asInt())) + && jsonNode + .decimalValue() + .compareTo(BigDecimal.valueOf(jsonNode.floatValue())) + == 0)) .build(); } diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/util/RowJsonTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/util/RowJsonTest.java index bd965e85c43e..fda8fea5480c 100644 --- a/sdks/java/core/src/test/java/org/apache/beam/sdk/util/RowJsonTest.java +++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/util/RowJsonTest.java @@ -568,6 +568,13 @@ public void testSupportedLongConversions() throws Exception { public void testSupportedFloatConversions() throws Exception { testSupportedConversion(FieldType.FLOAT, FLOAT_STRING, FLOAT_VALUE); testSupportedConversion(FieldType.FLOAT, SHORT_STRING, (float) SHORT_VALUE); + + // Integral literals a float still represents exactly: 2^24 is the largest contiguous one, + // and Integer.MIN_VALUE is exact because it is a power of two. Both are accepted today and + // must stay accepted -- they are what keeps the fix from being an across-the-board reject. + testSupportedConversion(FieldType.FLOAT, "16777216", 16777216.0f); + testSupportedConversion( + FieldType.FLOAT, String.valueOf(Integer.MIN_VALUE), (float) Integer.MIN_VALUE); } @Test @@ -686,10 +693,20 @@ public void testUnsupportedDoubleConversions() throws Exception { testUnsupportedConversion(FieldType.DOUBLE, LONG_STRING); // too large to fit } - // The two cases below get a method each on purpose. testUnsupportedConversion relies on the + // The three cases below get a method each on purpose. testUnsupportedConversion relies on the // ExpectedException rule, which is satisfied by the first exception to leave the test method, // so a second call in the same body never runs and the assertion would be silently dead. + @Test + public void testUnsupportedFloatConversionAtIntegerMaxValue() throws Exception { + // Integer.MAX_VALUE, and the guard against "fixing" this extractor with an int round-trip. + // (float) 2147483647 rounds up to 2147483648, and narrowing 2147483648 back to int + // saturates at Integer.MAX_VALUE, so the round-trip wrongly accepts it and stores + // 2147483648.0. INT_STRING in testUnsupportedFloatConversions is far smaller and does not + // exercise this -- and, being the fourth call in that method, never runs anyway. + testUnsupportedConversion(FieldType.FLOAT, "2147483647"); + } + @Test public void testUnsupportedDoubleConversionJustPastContiguousRange() throws Exception { // 2^53 + 1, the first integer a double cannot represent.