[BEAM-9615] Add initial Schema to Go conversions. #12471
Conversation
|
R: @youngoli |
youngoli
left a comment
youngoli
left a comment
There was a problem hiding this comment.
Looks good, just some small comments not worth blocking on.
| }, nil | ||
| case reflect.Interface, reflect.Chan, reflect.UnsafePointer, reflect.Complex128, reflect.Complex64: | ||
| return nil, errors.Errorf("unable to convert unsupported type %v to schema", ot) | ||
| default: // must be an atomic type |
There was a problem hiding this comment.
Reviewed the change, and the full error propogation is even better than my original suggestion, so two thumbs up for that.
But I think you missed my suggestion to merge these unsupported type and default cases. I.E. combine them into one default case that tries using reflectTypeToAtomicTypeMap, and if that fails, then it's an unsupported type.
Having a specific case for unsupported types seems brittle and doesn't serve much of a purpose other than having a slightly different error message than the one under default. If one of these types gets added, this is just one more spot that needs to be changed and will break if you forget.
Obviouslt it's not so important it needs an immediate fix, but I'd say it's worth bundling into whatever schema PR is coming next.
2 participants
Adds initial Schema to Go type conversions and vice versa.
In particular, handles the "easy" stuff: basic struct to Row conversion, Slices, and maps, basic field name conversions.
Doesn't implement struct+schema compatibility comparisons, which are necessary at pipeline construction time, however, this is more than sufficient to allow other immediately incoming code to unlock the row coder tests in standard_coders.yaml.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-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.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.