[
https://issues.apache.org/jira/browse/FLINK-1520?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14590633#comment-14590633
]
ASF GitHub Bot commented on FLINK-1520:
---------------------------------------
Github user vasia commented on the pull request:
https://github.com/apache/flink/pull/847#issuecomment-112954221
Hey @shghatge,
this is a great first try, you got the logic right and I really like the
detailed javadocs ^^
I left a few inline comments, which should be easy to fix.
Let me also elaborate a bit on some general guidelines:
- Code formatting: we don't really have a strict Java code style, but there
a few things you can improve. For your code to be readable, it is nice to leave
a space after the commas separating arguments. For example `myMethod(arg1,
arg2, arg3)` instead of `myMethod(arg1,arg2,arg3)`.
We usually separate the closing of a parenthesis and the opening of a curly
bracket with a space, i.e. `myMethod() { ... }` instead of `myMethod(){ ... }`.
Also, try to avoid adding new lines if they are not needed.
Regarding the types missing, this is not creating an error, but gives a
warning. You can turn on warning notification settings in your IDE to avoid
this.
- I like it that you added separate methods `includeFields` methods` for
vertices and edges. It would probably make sense to do the same for the rest of
the methods. For example, you might want to skip the first line in the edges
file, but not in the vertices file. Right now, you are forced to either do both
or none. Alternatively, we could add parameters to the existing methods, to
define the behavior for edges and vertices files separately. For example
`public GraphCsvReader lineDelimiter(String VertexDelimiter, EdgeDelimiter)`.
What do you think?
- Finally, in order to catch issues like the one with the null
`VertexReader`, you should always try to test as much functionality you have
added as possible. In this case, it would be a good idea to add a test reading
from edges only and some tests for the different methods you have added.
Let me know if you have questions!
> Read edges and vertices from CSV files
> --------------------------------------
>
> Key: FLINK-1520
> URL: https://issues.apache.org/jira/browse/FLINK-1520
> Project: Flink
> Issue Type: New Feature
> Components: Gelly
> Reporter: Vasia Kalavri
> Assignee: Shivani Ghatge
> Priority: Minor
> Labels: easyfix, newbie
>
> Add methods to create Vertex and Edge Datasets directly from CSV file inputs.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
