-
Notifications
You must be signed in to change notification settings - Fork 21
Batch csv #130
base: master
Are you sure you want to change the base?
Batch csv #130
Conversation
moves most of the CSV logic out of cmd/picsv and into csv package. refactor to be a bit cleaner and get ready to handle multiple sources of CSV data.
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.
In general, looks good to me. I posted a few minor comments, but i think this is good for at least "beta" release. A lot to digest for critical review
return batch, pc, nil | ||
} | ||
|
||
func openFileOrURL(name string) (io.ReadCloser, error) { |
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.
Its probably not worth the effort but,It would be nice if we provided a more generic storage interface to the various storage interfaces, kinda like https://github.com/kahing/goofys.
|
||
type SourceField struct { | ||
// TargetField is the Pilosa field that this source field should map to. | ||
TargetField string `json:"target-field"` |
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.
Minor: TargetFieldName would provide a little clarity. For some reason Seeing SourceField and TargetField together in the code made me focus on the fact that those are not the same type. Or alternatively SourceFieldArgs?
remove unnecessary OptFieldTypeBool()s
to command main instead of having on PDK main
add picsv command for importing CSV data using the experimental batch ingestion interface