You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2026/10/01 11:30:59 UTC
[PR] fix(expression): return a parse error for non-string type nodes in expression JSON [iceberg-cpp]
LuciferYang opened a new pull request, #982:
URL: https://github.com/apache/iceberg-cpp/pull/982
## What
Expression JSON deserialization in `src/iceberg/expression/json_serde.cc` called `json[kType].get<std::string>()` (and one `json[kTerm].get<std::string>()`) guarded only by `is_object()` and `contains(kType)`. A non-string `"type"`/`"term"` made nlohmann throw `json::type_error.302`, which escaped these `Result`-returning functions (the expression parse chain has no `try`/`catch`) and terminated the caller instead of returning an error. This is the same failure mode as the merged #857.
It is reachable, not test-only. Table-metadata parsing hits the type-aware `LiteralFromJson` through `FieldFromJson`'s `initial-default`/`write-default` handling, and REST responses hit the expression parsers through the scan-metrics report filter and the residual/partition/plan filters, all via `ICEBERG_ASSIGN_OR_RAISE`, which forwards a `Result` error but not a thrown exception.
Closes #979.
## How
Each of the six `get<std::string>()` sites now checks `is_string()` first and returns `JsonParseError` on a non-string node, mirroring the sibling `OperationTypeFromJson`. Valid string input is unaffected: the added `is_string()` sits in a short-circuit `&&`, so a well-formed node evaluates exactly as before, and the guard only rejects input that previously threw.
## Testing
`NonStringTypeIsParseError` in `expression_json_test.cc` covers all six guards: a non-string `"type"` at the top level (`ExpressionFromJson`), on an and/or node, on a predicate's term node (routed through the transform-term check and then the named-reference parser), a non-string reference `"term"`, and a non-string `"type"` on both the untyped and the type-aware `LiteralFromJson` overloads. Each case returns `kJsonParseError`; without the fix the corresponding input throws `json::type_error.302` out of the `Result`-returning function. Removing any single guard makes one of these cases throw, so no guard is left unpinned.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(expression): return a parse error for non-string type nodes in expression JSON [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #982:
URL: https://github.com/apache/iceberg-cpp/pull/982
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(expression): return a parse error for non-string type nodes in expression JSON [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #982:
URL: https://github.com/apache/iceberg-cpp/pull/982#issuecomment-6097839202
> LGTM.
>
> Agent alerted me there's one other similar failure issue during the code review:
>
> https://github.com/apache/iceberg-cpp/blob/b4eecba1e2a9ca3ef9926f07d5a76b0f85981f94/src/iceberg/json_serde.cc#L496-L505
>
> 's TypeFromJson accesses `json[kElement]` directly without check its existence, which also triggers SIGABRT. It should follow the MapTypeFromJson's [practice](https://github.com/apache/iceberg-cpp/blob/b4eecba1e2a9ca3ef9926f07d5a76b0f85981f94/src/iceberg/json_serde.cc#L510). would you mind to submit a follow-up pr as well?
Yes, will fix in a separate PR.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(expression): return a parse error for non-string type nodes in expression JSON [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #982:
URL: https://github.com/apache/iceberg-cpp/pull/982#issuecomment-6125514779
Thanks @wgtmac @kamcheungting-db @advancedxy
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(expression): return a parse error for non-string type nodes in expression JSON [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #982:
URL: https://github.com/apache/iceberg-cpp/pull/982#issuecomment-6098266782
@advancedxy Opened #997 for `ListTypeFromJson`. I went through the other const `json[key]` reads under `src/iceberg` and that was the only one that didn't check the key exists first.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(expression): return a parse error for non-string type nodes in expression JSON [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #982:
URL: https://github.com/apache/iceberg-cpp/pull/982#issuecomment-6109424451
Thanks @LuciferYang for fixing this and @advancedxy for the review!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org