Conversation
apilloud
left a comment
apilloud
left a comment
There was a problem hiding this comment.
I learned lots of fancy java reviewing this. LGTM.
| */ | ||
| @AutoValue | ||
| public abstract static class FieldType implements Serializable { | ||
| // Returns the type of this field. |
There was a problem hiding this comment.
All these comment changes seem unrelated to a Json Deserialzier. Separate commit?
There was a problem hiding this comment.
Sure, removed it from this PR. Missing javadoc was confusing so i added it while i was at it.
| .collect(Row.toRow(targetRowSchema)); | ||
| } | ||
|
|
||
| private static Object getFieldValue( |
There was a problem hiding this comment.
nit: Following this is kind of confusing. It seems like this function should really part of your mapToObj call above.
There was a problem hiding this comment.
I agree. Did this before schemas exposed fields list. Updated
| "Field " + currentFieldName + " is not present in the JSON object"); | ||
| } | ||
|
|
||
| if (currentValueNode.isNull()) { |
There was a problem hiding this comment.
I didn't see a test that covered null values. Might be worth testing.
akedin
force-pushed
the
row-json-deserializer
branch
2 times, most recently
from
00b79ef to
ae5e86d
Compare
kennknowles
left a comment
kennknowles
left a comment
There was a problem hiding this comment.
This is very nice. I have two high-level comments:
- It might be preferable to have the conversion driven by the expected type in the schema instead of by the JSON type. It doesn't seem to be a problem, though.
- Can you have tests for out-of-bounds numerical constants? I may have just missed them. The reason I ask is to deliberately focus on the places where the JSON is valid and almost fits the schema, but there is a minor mismatch in the data models.
That's what happens. These checks look at the schema type, and primitive value look up also happens by schema type name.
Working on it |
Add basic Deserializer to allow converting Json objects to Rows.
akedin
force-pushed
the
row-json-deserializer
branch
from
ae5e86d to
9386a44
Compare
|
@kennknowles added test for overflowing integer numbers |
|
Ah, sorry I misread that. Then the choice of when to reject and when to wrap and when to lose precision is a bit complicated. I think we might need a design doc and to discuss a little bit on dev@ as a next gen of the schema design, but we can move forward with almost anything for now. |
Extract and validate json primitive types for stricter compatibility with java types
|
@kennknowles added stricter validation, does this approach look better? Few quirks:
At this point I think that having a more restricting approach is better even while we don't have a robust type system in Schemas:
|
|
run java precommit |
|
Yes, I like this. If we do decide to make the extractors less strict, the structure of things can stay the same. |
3 participants
Add basic Deserializer to allow converting Json objects to Rows.
Doesn't support nullables at the moment. Isn't wired up to SQL yet.
Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue.mvn clean verifyto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.