[Cherry-pick to branch-1.3] [#12598] fix(common): Reject out-of-range numeric statistic values (#12599) - #12874
Merged
roryqi merged 1 commit intoSep 3, 2026
Conversation
…ues (apache#12599) ### What changes were proposed in this pull request? Add two range guards to `JsonUtils.getStatisticValue`: - integral branch: `JsonNode.canConvertToLong()` before `asLong()` - floating-point branch: `Double.isFinite()` on the result of `asDouble()` Both reject through `Preconditions.checkArgument`, matching how the other read paths in this file signal bad input (`readFunctionArg` and the partition reader both throw `IllegalArgumentException`). The change also drops the checked-exception plumbing around the terminal branch. It threw `UnsupportedEncodingException`, a character-encoding error used to report a bad JSON node type, which forced `getStatisticValue` to declare `throws IOException`, which in turn forced the object branch to launder that exception out of a lambda through a bare `RuntimeException`. None of it was ever reachable, before or after this change: JSON text can only produce node types the method already handles. The branch is reachable through `ObjectMapper.convertValue` with an embedded binary node, which is what the new test for it uses. It now throws `IllegalArgumentException`, and the `throws` clause and the `try/catch` are gone. Two `if (value != null)` checks in the recursive branches are removed as well. `getStatisticValue` never returns null: every branch either returns a `StatisticValues` instance or throws. ### Why are the changes needed? A statistic value past the 64-bit range was silently replaced by a different number, with the sign flipped in some cases. `9223372036854775808` was stored as `-9223372036854775808`, and `123456789012345678901234567890` as `-4362896299872285998`. An out-of-range floating-point literal became `Infinity`, which the serializer writes back out as the JSON string `"Infinity"`, so the value returned as a `StringValue` on the next round trip. `StatisticsUpdateRequest.validate()` cannot catch this: it only checks for a null value, and it runs after Jackson has built the map, by which point the truncated `long` is all that is left. The deserializer is the only place where the information needed to detect the loss still exists. `StatisticValue` has no BigInteger or BigDecimal type, so there is no lossless representation to fall back to, and failing the request is better than storing a wrong number. Fix: apache#12598 ### Does this PR introduce _any_ user-facing change? Yes. `PUT /metalakes/{metalake}/objects/{type}/{fullName}/statistics` and its `/partitions` variant now return 400 for a numeric statistic value outside the `long` range, or a floating-point value that is not finite. They previously returned 200 and stored a wrong number. No stored data becomes unreadable. The serializer can only emit in-range longs and finite doubles (a non-finite double goes out as the quoted string `"Infinity"`), so nothing already persisted trips the new guards. Rows already corrupted by the old behaviour keep their wrong value; this change does not repair them. No API signatures, property keys, or configuration change. ### How was this patch tested? Four test methods in `TestJsonUtils`. All four were confirmed to fail against the pre-fix code by reverting `JsonUtils.java` and re-running, not by inspection: - `testStatisticValueRejectsOutOfRangeIntegral` — both 64-bit boundaries are still accepted; one past each boundary and two far outside are rejected, including nested inside a list and inside an object. - `testStatisticValueRejectsNonFiniteFloatingPoint` — `±1.5E400`, asserting the full message including the rendered `Infinity` / `-Infinity`. The message reports the parsed double rather than echoing the literal, because the node Jackson hands the deserializer already holds the infinity. - `testStatisticsUpdateRequestRejectsOutOfRangeValue` — the real request-body shape, where the value is `Map` content and Jackson wraps the rejection into `JsonMappingException`, which the server maps to 400. It uses a bare `ObjectMapper` so the assertion rests on the DTO's `@JsonDeserialize(contentUsing = ...)` annotation rather than on a module the test registered; removing that annotation makes the test fail. - `testStatisticValueRejectsUnsupportedNodeType` — the terminal branch. It asserts `assertNull(e.getCause())`, because `ObjectMapper.convertValue` relaunders a deserializer `IOException` into an `IllegalArgumentException` carrying the same message, so only the cause distinguishes our own rejection from the old checked exception. ``` ./gradlew :common:test :core:test :server:test :common:javadoc :common:spotlessCheck -PskipITs ``` passes. Follow-ups found while working on this, not included here to keep the change to one concern: - `StatisticValues.doubleValue(double)` accepts `Infinity` and `NaN`, so the write side can still produce a value this change now refuses to read back as a double. The root fix belongs in `api` and carries its own compatibility discussion. - `PartitionStatisticsUpdateDTO.validate()` has no per-entry null check, unlike `StatisticsUpdateRequest.validate()`. Jackson's `MapDeserializer` does not invoke a `contentUsing` deserializer for a `VALUE_NULL` content token, so a top-level JSON null reaches storage on that route. - `JdbcPartitionStatisticStorage.parseResultSet` catches `JsonProcessingException` to log the partition and statistic name; an unchecked `IllegalArgumentException` bypasses that handler.
yuqi1129
approved these changes
Sep 3, 2026
Code Coverage Report
Files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Cherry-pick commit
09e68277b2d8df18b397c2373a2ada2aef8ce04ffrom #12599 tobranch-1.3.This change:
Why are the changes needed?
Out-of-range numeric statistic values are silently converted and stored as different values in 1.3. For example, an integer larger than
Long.MAX_VALUEwraps to a negative value, while an oversized floating-point value becomesInfinity.This backport prevents silent statistic corruption by rejecting these values.
Backport: #12599
Does this PR introduce any user-facing change?
Yes. Out-of-range numeric statistic values now result in a
400 Bad Requestinstead of returning success and storing an altered value.No API signatures, properties, or stored-data formats are changed.
How was this patch tested?