PDGGK commented on PR #39744: URL: https://github.com/apache/beam/pull/39744#issuecomment-5296626238
Extended this to the sibling method in the same file — `floatValueExtractor` has the identical defect, and it would be odd to fix one and leave the other for a reviewer to notice. It validates an integral literal the same way the double branch did: ```java jsonNode.asInt() == (int) (float) jsonNode.asInt() ``` Narrowing an out-of-range float back to `int` **saturates**, so this is not the identity check it looks like. Measured across the boundary 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 single value it gets wrong: the float it passes through is 2147483648, narrowing that back to `int` saturates at 2147483647, the equality holds, and the extractor then stores **2147483648.0**. A document saying `2147483647` reads back as a different number with no error. Now compared through `BigDecimal`, the same way this PR already fixes `doubleValueExtractor`. `Integer.MIN_VALUE` and 2^24 stay accepted — both are exactly representable — so this is not an across-the-board tightening; there are now two supported-conversion assertions guarding exactly that. The unsupported case gets its own test method, for the reason already noted in this PR: `ExpectedException` is satisfied by the first exception to leave the method. That is also why the existing `testUnsupportedFloatConversions` cannot cover it — its `INT_STRING` call is the fourth statement in that body and has never executed. Reverting only the float validator fails `testUnsupportedFloatConversionAtIntegerMaxValue` and nothing else (79 tests, 1 failed). `spotlessJavaCheck`, `checkstyleMain` and `checkstyleTest` on `:sdks:java:core` are clean. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
