New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BEAM-8484] RowJsonDeserializer/ToJson cleanup #9757
Conversation
R: @apilloud |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I read the description, I see this is just a refactor. Oops.
LGTM
sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowJson.java
Outdated
Show resolved
Hide resolved
|
||
private static Object extractJsonPrimitiveValue(FieldValue fieldValue) { | ||
try { | ||
return JSON_VALUE_GETTERS.get(fieldValue.typeName()).extractValue(fieldValue.jsonValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this might be more readable as a switch statement, but it looks like you are just moving it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah agreed a switch would be better. Do you want me to change that for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is fine as is.
Run Java PreCommit |
ping @apilloud |
Looks like you have a merge commit in your history. You should set |
I usually don't use |
I thought we preferred rebasing, but I see nothing requiring it in the committer guide. You do need a JIRA though. Can you add a JIRA to the title? |
80d741d
to
0cf8ae8
Compare
Rebased and added a JIRA |
RowJsonDeserializer
andRowJsonSerializer
intoRowJson
as suggested in [BEAM-8354] AddRowJsonSerializer
#9732 (review)ToJson#of
public andnew ToJson()
privatePost-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.