feat: add json serde for REST Catalog request/response models - #251
Merged
Conversation
HeartLinked
force-pushed
the
feat/catalog-json
branch
3 times, most recently
from
October 15, 2025 08:39
ca9db4e to
c27891e
Compare
HeartLinked
marked this pull request as ready for review
October 15, 2025 09:04
HeartLinked
force-pushed
the
feat/catalog-json
branch
from
October 20, 2025 12:13
5af1ab9 to
ae5190a
Compare
wgtmac
requested changes
Oct 22, 2025
wgtmac
reviewed
Oct 22, 2025
HeartLinked
force-pushed
the
feat/catalog-json
branch
from
October 24, 2025 10:42
e393dde to
d3f9b81
Compare
HeartLinked
force-pushed
the
feat/catalog-json
branch
from
October 24, 2025 10:56
d3f9b81 to
8444ef8
Compare
wgtmac
requested changes
Oct 27, 2025
wgtmac
left a comment
Member
There was a problem hiding this comment.
Thanks for working on this! I mainly have two feedbacks other than those inline comments:
- All Java request and reposes classes inherit from
RESTMessagebelow with avalidatemethod. I think this is important because these objects may be created out of our control and must be validated before use.
/** Interface to mark both REST requests and responses. */
public interface RESTMessage {
/**
* Ensures that a constructed instance of a REST message is valid according to the REST spec.
*
* <p>This is needed when parsing data that comes from external sources and the object might have
* been constructed without all the required fields present.
*/
void validate();
}I'd propose to add a iceberg/catalog/rest/validator with interface like below for every request and response:
struct Validator {
static Status Validate(const ListNamespacesResponse& resp);
...
};- Interoperability is our top priority. I have followed all Java test cases for TableMetadata serde by adding them to
src/iceberg/test/metadata_serde_test.cc. There are a lot of test in the Java repo likeorg/apache/iceberg/rest/responses/TestListNamespacesResponse.javawhich contains both good and bad cases. It would be good to include them while implementing each objects.
wgtmac
reviewed
Oct 27, 2025
wgtmac
reviewed
Oct 27, 2025
wgtmac
reviewed
Nov 3, 2025
HeartLinked
force-pushed
the
feat/catalog-json
branch
from
November 3, 2025 09:14
4d34bf8 to
e19f740
Compare
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.
This PR depends on and should be merged after #230. #249 #245 #248