[FLINK-2785] [gelly] implements fromCsvReader for gelly-scala

[vasia]

This is the last method missing from the Scala Graph implementation.
In order to be as close as possible to the Java implementation, I implemented one method where different options are given with optional parameters. This resulted into the method exceeding the allowed max number of parameters, as defined in the scala checkstyle. I decided to disable the checkstyle for this particular method, because most of the parameters are optional and are there mainly for completeness. In the common case, a user would only give the path and delimiters. If you think there's a better way to do this without having to disable the checkstyle, let me know!

[fhueske]

How about we rename this parameter to sth like readVertices :Boolean and making it mandatory.
If readVertices is set, pathVertices must be specified. It also implies that there are vertex values (otherwise it would make no sense to read a vertex file, right?).

[fhueske]

Hi, you might have noticed, I got a bit confused about the vertexValue parameter and how it affects the graph creation. I made a proposal inline to rename that parameter and making it mandatory. Let me know what you think. The documentation should discuss the vertexValue in more detail.

Otherwise the PR looks good.

[vasia]

Thanks so much for the review @fhueske!
I agree with your comments. I will rename the parameter to readVertices, make it mandatory, and better explain it on the docs. I also like the suggestion for edgeValue -> hasEdgeValues.

[chiwanpark]

I'm worrying about the name of method fromCsvReader. It seems that the method should receive CsvReader object and read graph data from the reader object. Why don't we use other name such as fromCsvFile?

[vasia]

Hey @chiwanpark,
this is how the Java method works, i.e. creates a CsvReader object and returns the graph when calling the appropriate types methods. I only kept this name in Scala to be consistent with the Java API.

[chiwanpark]

Okay, I understand it.

[fhueske]

Should we change the order of the parameters and move the most common once first? This will allow to specify some parameters without names. How about:

• env (mandatory)
• pathEdges (mandatory)
• readVertices
• pathVertices
• hasEdgeValues
• ... (the others in the current order)

[fhueske]

Thanks for the fast update! Good to merge.
I added a comment about the parameter order that could be addressed if you agree.

[vasia]

Great, thanks! I will re-order the parameters and merge.

[vasia]

There was an error in RandomSamplerTest in one of the builds. I'll open a JIRA and merge this.