fix: reject out-of-range integers when parsing JSON literals - #878
Open
LuciferYang wants to merge 2 commits into
Open
fix: reject out-of-range integers when parsing JSON literals#878LuciferYang wants to merge 2 commits into
LuciferYang wants to merge 2 commits into
Conversation
nlohmann reports unsigned integers as is_number_integer(), and get<int64_t>() converts values above INT64_MAX silently instead of throwing. So an integer beyond the signed range was accepted as a literal: the kInt branch ran its int32 range check on the already-wrapped value, and the kLong branch had no range check at all. 18446744073709551615 parsed as Literal::Int(-1) rather than returning a parse error. Add a GetInt64Checked helper that rejects unsigned values above INT64_MAX before the conversion, and use it in the kInt and kLong branches of the type-aware parser plus the untyped overload. This matches Java, where SingleValueParser and ExpressionParser guard the same paths with canConvertToInt()/canConvertToLong().
The out-of-range check is a conjunction: is_number_unsigned() plus a comparison against INT64_MAX. Neither accept-side had a test, so dropping either half went unnoticed. Add LongMax (an unsigned node exactly at INT64_MAX, where the ULL suffix is load-bearing), LongMin and IntNegative for the signed path, and an untyped negative case. Also cover the int32 narrowing that follows the shared guard on the kInt path, move the helper into an anonymous namespace so it stops taking an external symbol, and use one wording for both out-of-range messages.
There was a problem hiding this comment.
Pull request overview
This PR hardens JSON literal parsing in LiteralFromJson to correctly reject integral JSON values that exceed the signed 64-bit range, preventing silent wrap/truncation when converting nlohmann unsigned integer nodes to int64_t. This aligns C++ behavior with Java parsers and avoids cross-engine inconsistencies when reading table metadata defaults.
Changes:
- Add
GetInt64Checkedto explicitly reject unsigned integer JSON nodes greater thanINT64_MAXbefore converting toint64_t. - Route both typed (
kInt,kLong) and untyped integral parsing throughGetInt64Checked, and unify the out-of-range error wording. - Add regression tests covering unsigned overflow rejection, int32 narrowing, and negative-literal acceptance (typed and untyped).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/iceberg/expression/json_serde.cc | Introduces GetInt64Checked and uses it in typed/untyped integral literal parsing to reject out-of-range unsigned integers. |
| src/iceberg/test/expression_json_test.cc | Adds targeted test coverage for overflow rejection, boundary acceptance, and int32 narrowing behavior. |
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
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
LiteralFromJsonaccepted JSON integers beyond the signed 64-bit range and produced a wrong literal instead of a parse error. nlohmann reports unsigned integers asis_number_integer(), andget<int64_t>()converts values above INT64_MAX silently rather than throwing, so thekIntbranch ran its int32 range check on the already-wrapped value and thekLongbranch had no range check at all.18446744073709551615parsed asLiteral::Int(-1), and9223372036854775808asLiteral::Long(INT64_MIN). The untyped overload had the same hole.The live consequence is on table metadata:
initial-default/write-defaultgo through this parser,ValidateDefaulthas no integer range check, and the value is later materialized into a returned column. A metadata file that Java rejects reads back as-1in C++. The expression path is latent for now, since no reader evaluatesReaderOptions::filteryet.Fixes #877.
How
Added
GetInt64Checked, which rejects unsigned nodes above INT64_MAX before the conversion, and called it from thekIntbranch, thekLongbranch, and the untyped overload. This mirrors Java, whereSingleValueParserandExpressionParserguard the same paths withcanConvertToInt()/canConvertToLong().The
is_number_unsigned()half of the guard is load-bearing:get<uint64_t>()on a negative node yields its two's-complement value, which compares above INT64_MAX and would reject every negative literal. Both halves now have accept-side tests.Also in this PR, both on lines the fix touches: the int32 narrowing on the
kIntpath gained the coverage it never had, and the two out-of-range messages now use one wording instead of saying "int" in one place and "long" in the other.Testing
expression_test526 tests and the fullctestsuite (18/18) pass. Each new test was checked against a mutation of the code it guards:>changed to>=in the guard: onlyLongMaxfails (theULLsuffix there is load-bearing, a signed INT64_MAX node would skip the unsigned branch entirely).is_number_unsigned() &&dropped:LongMin,IntNegativeandAcceptsNegativeIntegerUntypedfail.IntAboveInt32MaxandIntBelowInt32Minfail.Verified fail-without / pass-with for the three overflow-rejection cases as well.
Out of scope
GetTypedJsonValueinsrc/iceberg/util/json_util_internal.htruncates out-of-range integers the same way, soFieldFromJson({"id": 2147483648, ...})yieldsfield_id = -2147483648silently. That helper has on the order of 80 call sites and is left for a follow-up rather than widened into this PR.