refactor(serde): return Result from ToJson schema/type/metadata serializers#785
Merged
Merged
Conversation
…lizers Single-value (Literal) serialization can fail, and upcoming v3 column default values (apache#730) invoke it from schema serialization. Switch the schema/type/metadata ToJson serializers (and the REST request/response serializers that embed them) from returning bare nlohmann::json to Result<nlohmann::json>, propagating errors via ICEBERG_ASSIGN_OR_RAISE instead of forcing callers to throw. Behavior is unchanged: every conversion still succeeds today; this only changes the return type so the column-defaults PR can serialize default values without throwing. - ToJson for SchemaField/Type/Schema/TableMetadata/TableUpdate, plus REST CreateTableRequest/CommitTableRequest/LoadTableResult/CommitTableResponse. - The shared ToJsonList template stays bare (also used by infallible types); TableMetadata serializes its schema list with an explicit loop. - Callers bottom out at existing Result boundaries (ToJsonString wrappers, rest_catalog.cc, TableMetadataUtil::Write).
wgtmac
approved these changes
Jun 27, 2026
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
2 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
Change the schema/type/metadata
ToJsonserializers from returning barenlohmann::jsontoResult<nlohmann::json>:json_serde:ToJson(SchemaField | Type | Schema | TableMetadata | TableUpdate)ToJson(CreateTableRequest | CommitTableRequest | LoadTableResult | CommitTableResponse)Errors propagate via
ICEBERG_ASSIGN_OR_RAISE; callers bottom out at the existingResult-returning boundaries (ToJsonString,rest_catalog,TableMetadataUtil::Write).Why
Preparation for v3 column default values (#730 / #746). Single-value (
Literal) serialization is fallible, and column defaults invoke it from schema serialization, so these serializers need to propagate the error instead of throwing. Splitting it out keeps the feature PR focused on the feature.Notes
ToJsonListtemplate stays bare (it also serializes infallible types such as partition specs and snapshots);TableMetadataserializes its schema list with an explicit loop.ICEBERG_DECLARE_JSON_SERDEmacro is unchanged; the four schema/metadata-bearing models are declared explicitly so only theirToJsonreturn type differs.Testing
No behavior change; existing tests are adapted to the new return type. Full build and
ctestpass locally.