This is an automated email from the ASF dual-hosted git repository.
stankiewicz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/beam.git
The following commit(s) were added to refs/heads/master by this push:
new 8abe4a39a4a Accept integral JSON values that a double represents
exactly (#39744)
8abe4a39a4a is described below
commit 8abe4a39a4a8dfe88cd5f0ccb8abf1b12506013e
Author: ZIHAN DAI <[email protected]>
AuthorDate: Wed Sep 2 22:43:17 2026 +1000
Accept integral JSON values that a double represents exactly (#39744)
* 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.
* 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 | 22 +++++++++--
.../java/org/apache/beam/sdk/util/RowJsonTest.java | 44 ++++++++++++++++++++++
2 files changed, 62 insertions(+), 4 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 2179b20010d..e769212fce7 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 @@ class RowJsonValueExtractors {
|| (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();
}
@@ -132,10 +139,17 @@ class RowJsonValueExtractors {
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 81f69b62c53..fda8fea5480 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 class RowJsonTest {
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
@@ -575,6 +582,14 @@ public class RowJsonTest {
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 +693,35 @@ public class RowJsonTest {
testUnsupportedConversion(FieldType.DOUBLE, LONG_STRING); // too large
to fit
}
+ // 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.
+ 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 {
