-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
dolt table import -r #86
Conversation
db09aa5
to
170e168
Compare
ba00761
to
10aacb1
Compare
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.
Are these your first go changes? Looking pretty good! Most of the feedback comes down to how the verification is done. Take a look at my comments and let me know if you have any questions.
@@ -77,8 +77,13 @@ func NewJSONReader(nbf *types.NomsBinFormat, r io.ReadCloser, info *JSONFileInfo | |||
return nil, err | |||
} | |||
|
|||
ableToDecode := true |
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 was tricky to verify the schema for JSON because either a schema file is required or the schema from the existing table is used. When decoding the JSON rows here, an error is returned if the rows don't match the existing schema. This was not being handled before so I added an AbleToDecode
property to JSONFileInfo
that tracks whether the rows were decoded or not and can be used in the VerifySchema
method. Let me know if you think there's a better way to do this - I couldn't directly compare the schemas here like I could for CSV and XLSX files.
f78e463
to
8afef26
Compare
8afef26
to
93dd368
Compare
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.
lgtm
One thing I would ask is that you go and find all the VerifySchema declarations and add comments to them. We are trying to have comments on all exported methods in this codebase:
d5971d4
to
59536a4
Compare
Fixes #76
Replaces existing table with the contents of the file while preserving the original schema