You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "PDGGK (via GitHub)" <gi...@apache.org> on 2026/08/13 12:12:02 UTC
[PR] Accept integral JSON values that a double represents exactly [beam]
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Accept integral JSON values that a double represents exactly [beam]
Posted by "PDGGK (via GitHub)" <gi...@apache.org>.
PDGGK commented on PR #39744:
URL: https://github.com/apache/beam/pull/39744#issuecomment-5294494390
The one red check is `beam_PreCommit_Java_IOs_Direct`, failing in a module this PR does not touch:
```
JmsIOTest > testCheckpointMarkSafety[with client class class org.apache.qpid.jms.JmsConnectionFactory] FAILED
> Task :sdks:java:io:jms:test FAILED
70 tests completed, 1 failed
```
This PR changes `sdks/java/core/.../util/RowJsonValueExtractors.java` and its test; the failure is a timing-sensitive JMS checkpoint test in `sdks/java/io/jms`, 69 of 70 in that module passing.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Accept integral JSON values that a double represents exactly [beam]
Posted by "PDGGK (via GitHub)" <gi...@apache.org>.
PDGGK commented on PR #39744:
URL: https://github.com/apache/beam/pull/39744#issuecomment-5329171694
Run Java_IOs_Direct PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Accept integral JSON values that a double represents exactly [beam]
Posted by "PDGGK (via GitHub)" <gi...@apache.org>.
PDGGK commented on PR #39744:
URL: https://github.com/apache/beam/pull/39744#issuecomment-5281881346
The one red check is `beam_PreCommit_Java_IOs_Direct`, and it is a known-flaky JMS test rather than anything from this PR:
```
JmsIOTest > testCheckpointMarkSafety[with client class org.apache.qpid.jms.JmsConnectionFactory] FAILED
70 tests completed, 1 failed
> Task :sdks:java:io:jms:test FAILED
```
- #30225 is **still open** and tracks exactly this: *"[Failing Test]: JmsIOTest. testCheckpointMark flaky"*. Its own description cites an example run from "a PR unrelated to Jms".
- #26203 (JMS tests flaky due to port occupied) and #36976 (loosening an immediate-advance assertion in the same area) are earlier rounds of the same thing.
- This PR changes two files, both `sdks/java/core/.../util/RowJson*`. `grep -rl RowJson sdks/java/io/jms/src` returns nothing, so there is no path from this diff to the JMS reader's checkpoint marks.
Everything else is green: 21 passing, 2 skipped. `:sdks:java:core:test --tests '*RowJsonTest*'` is 78/78 locally, and `spotlessCheck` / `checkstyleMain` / `checkstyleTest` pass.
Per the bot's instructions, overriding so this can get a look.
assign set of reviewers
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Accept integral JSON values that a double represents exactly [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39744:
URL: https://github.com/apache/beam/pull/39744#issuecomment-5425151165
Reminder, please take a look at this pr: @ahmedabu98
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Accept integral JSON values that a double represents exactly [beam]
Posted by "PDGGK (via GitHub)" <gi...@apache.org>.
PDGGK commented on PR #39744:
URL: https://github.com/apache/beam/pull/39744#issuecomment-5294497831
assign set of reviewers
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Accept integral JSON values that a double represents exactly [beam]
Posted by "PDGGK (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Accept integral JSON values that a double represents exactly [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39744:
URL: https://github.com/apache/beam/pull/39744#issuecomment-5281893402
Assigning reviewers:
R: @chamikaramj for label java.
Note: If you would like to opt out of this review, comment `assign to next reviewer`.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Accept integral JSON values that a double represents exactly [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39744:
URL: https://github.com/apache/beam/pull/39744#issuecomment-5281800556
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment `assign set of reviewers`
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Accept integral JSON values that a double represents exactly [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39744:
URL: https://github.com/apache/beam/pull/39744#issuecomment-5294509741
Assigning reviewers:
R: @ahmedabu98 for label java.
Note: If you would like to opt out of this review, comment `assign to next reviewer`.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Accept integral JSON values that a double represents exactly [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39744:
URL: https://github.com/apache/beam/pull/39744#issuecomment-5454969164
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @chamikaramj for label java.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Accept integral JSON values that a double represents exactly [beam]
Posted by "stankiewicz (via GitHub)" <gi...@apache.org>.
stankiewicz merged PR #39744:
URL: https://github.com/apache/beam/pull/39744
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org