PDGGK opened a new pull request, #39744:
URL: https://github.com/apache/beam/pull/39744
Fixes #39743.
`doubleValueExtractor`'s validator compared `asLong()` against a round-trip
through `asInt()`:
```java
&& 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. `{"f": 1609459200000}` against a DOUBLE field fails
today. The guard on the line above is `canConvertToLong()`, so the `asInt()` is
at odds with its own precondition.
### Why not just swap `asInt()` for `asLong()`
Because that trades an over-rejection for silent corruption. Verified by
running it:
```
Long.MAX_VALUE = 9223372036854775807
(double) it = 9223372036854775808 <- rounds up to 2^63
(long)(double) it = 9223372036854775807 <- narrowing SATURATES, does
not overflow
naive round-trip ok = true <- so the check passes
```
The extractor would then store `9223372036854775808`. Comparing through
`BigDecimal` is exact, and is what the decimal branch immediately below already
does — so this makes the two branches consistent rather than introducing a new
idiom. (`compareTo(...) == 0`, not `.equals()`, per the `BigDecimalEquals`
ErrorProne check enabled in #38193.)
### Tests
Four supported cases added — epoch millis, its negative, 2^31 (the exact
boundary where the old truncation began), and 2^53 (the largest contiguous
integral double). Two rejections — 2^53+1 and `Long.MAX_VALUE`.
**Three-way check on the same tests:**
| variant | result |
|---|---|
| master (`asInt`) | fails `testSupportedDoubleConversions` — the
over-rejection |
| naive (`asLong`) | fails `testUnsupportedDoubleConversionAtLongMaxValue` —
the silent corruption |
| this (`BigDecimal`) | **78/78 pass** |
### One thing worth flagging separately
The two rejection cases get a test method each rather than being appended to
`testUnsupportedDoubleConversions`, and that is deliberate.
`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 executes**.
I checked how widespread that is while writing this: across the eight
`testUnsupported*Conversions` methods there are 45 `testUnsupportedConversion`
calls, and 37 of them are unreachable for this reason. Converting the helper to
`assertThrows` so they all run makes 6 of the 8 methods fail, so there is
something real behind them — but that is well outside the scope of this change
and I have not touched it. Happy to file it separately if that would be useful.
`spotlessCheck`, `checkstyleMain` and `checkstyleTest` pass.
--
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]
