Issue 7335 - Refactor AddTableRequestSerDe & tests#7693
Conversation
patchwork01
changed the title
patchwork01
added
the
needs-reviewer
|
Does the AddTableResponseSerDe not need updating? |
| } | ||
|
|
||
| @Override | ||
| public AddTableRequest deserialize(JsonElement jsonElement, java.lang.reflect.Type typeOfSrc, JsonDeserializationContext context) throws JsonParseException { |
There was a problem hiding this comment.
I think you don't need the java.lang.reflect.Type as that seems to be imported in this class
There was a problem hiding this comment.
I'm not sure how that happened, thanks.
| private static TableProperties readTableProperties(InstanceProperties instanceProperties, JsonObject object, JsonDeserializationContext context) { | ||
| Properties properties = context.deserialize(object.get("properties"), Properties.class); | ||
| Schema schema = context.deserialize(object.get("schema"), Schema.class); | ||
| if (properties == null) { |
There was a problem hiding this comment.
This can be moved above the schema desiralizing as there's no point in doing that if the properties is null
There was a problem hiding this comment.
I find the split points object in these tests confusing where certain tests set splitPoints to something but it isn't clear from the test that createAddTableRequest picks this up. Please add a second createAddTableRequest method that takes the split points in as a list and have the current one call that with List.of()
ca61688
removed
the
needs-reviewer
Spoke to @patchwork01 and they had looked at the response object and didn't believe there were changes to be made there. Approving for merge |
3 participants
Make sure you have checked all steps below.
Issue
Feature". Note that before an issue is finished, you can still make a pull request by raising a separate issue
for your progress.
Tests
Documentation
separate issue for that below.