[#551] csv graph collection import and export by klaudworks · Pull Request #976 · dbs-leipzig/gradoop

klaudworks · 2018-08-31T14:15:28Z

This pull request adds support to read and write graph collections using the csv data source and sinks.

p-f

Some comments about the code.

I think we should leave the "old" CSV format in the io package, maybe in a deprecated subpackage, but a utils.converter.deprecated.sources package doesn't seem fitting, since it's not actually a converter.

I would suggest:

Keep the old CSV format in the io package, make clear that it should not be used.
Add a converter from the old CSV format to the new.
Change the name from OldCSVDataSource to something more fitting, like CSVDataSourceWithoutGraphHeads (name is up for debate, but calling it "old" seems wrong, since it is currently the recommended format)
After this PR: Update Gradoop version to 0.6 since this is a major change.
Maybe create an Issue for all those CSV-related changes to summarize all changes to this format.

p-f · 2018-09-18T13:03:47Z

+ *
+ * The string needs to be encoded in the following format:
+ *
+ * edge-id;source-id;target-id;edge-label;value_1|value_2|...|value_n


Please put comments like this in a {@code ...} block

p-f · 2018-09-18T13:06:18Z

+   * @param limit resulting array length
+   * @return tokens
+   */
+  public String[] split(String s, int limit) {


This method can be protected, it should not be part of the API.

p-f · 2018-09-18T13:07:05Z

+ *
+ * The string needs to be encoded in the following format:
+ *
+ * vertex-id;vertex-label;value_1|value_2|...|value_n


{@code ...} block here too

p-f · 2018-09-18T13:08:32Z

+import org.gradoop.flink.io.impl.csv.metadata.MetaData;
+
+/**
+ * /**


Please remove this line (duplicate block-comment start).

p-f · 2018-09-18T13:09:44Z

+ * The datasource expects files separated by vertices and edges, e.g. in the following directory
+ * structure:
+ *
+ * csvRoot


Put those lines in a <pre><code> ... </code></pre> block to avoid unwanted newlines in Javadoc.

p-f · 2018-09-18T13:26:39Z

-      mapWriter(record, label);
+    String label = ((CSVElement) record).getLabel();
+    if (label.equals("")) {
+      throw new RuntimeException("IndexedCSVDataSink requires a label for every element.");


Maybe use a different Exception here, a RuntimeException seems too severe.

p-f · 2018-09-18T13:28:54Z

+    return f4;
  }

  public void setLabel(String label) {


Please use @Override here, since you declared that method in a new interface.
(Also not part of this PR, but please add short Javadoc comments for the other methods of this class.)

p-f · 2018-09-18T13:29:34Z

+public interface CSVElement {
+  /**
+   * Returns the elements label.
+   * @return label


Missing Javadoc comment for label.

p-f · 2018-09-18T13:30:15Z

+    f0 = id;
+  }
+
+  public String getLabel() {


Use @Override here too.

p-f · 2018-09-18T13:32:09Z

  @Test
  public void testPageRankExecution() throws Exception {
    LogicalGraph input = getSocialNetworkLoader().getLogicalGraphByVariable("g0");
-    input.print();


Oh I missed that, thanks 👍

p-f

When writing a LogicalGraph with a graph head having Properties, the Properties are written to CSV, but discarded in CSVDataSource.getLogicalGraph, because the ReduceCombination creates a new graph head.

p-f · 2018-09-18T14:32:54Z

+   */
  @Override
  public LogicalGraph getLogicalGraph() {
+    return getGraphCollection().reduce(new ReduceCombination());


Using a ReduceCombination will create a new graph head and therefore discard graph head properties.

The reduce combination used when reading a single logical graph is the way most data sources work.
I opened Issue #974 to discuss improvements. Regarding this pull request, I would keep it this way and change this behavior for all the affected data sources.

Okay, I forgot about that issue. So yes, leave it like this.

p-f · 2018-09-26T12:29:04Z

Please update from develop and fix the conflict in ReducePropertyMetaData

…mat and minor fixes

…for complex property types and clean indexed CSV paths of illegal characters

timo95 · 2018-09-27T15:24:27Z

Change CSV string representation of complex property types (fixes Change csv strings of complex property types (Map, List, Set) #985)
Replace illegal path characters in indexed CSV and set path toLowerCase (part of Gradoop fails to build on case-insensitive files systems (OSX/Windows) #762)

…al graph import

…doop into #551_support_graph_collection

galpha · 2018-09-28T09:37:22Z

+import org.apache.flink.api.java.tuple.Tuple3;
+
+/**
+ * Tuple representing an edge in a CSV file.


'an graphhead in a csv file'

galpha · 2018-09-28T09:43:00Z

Good job @2start and @timo95, after the docu fix i'll approve.

ChrizZz110

Some small changes.
I do not like the pattern to move deprecated classes in a package called "deprecated". I think the usual way is to mark the classes with the deprecated annotation and let them in the package thy belong to. But if you agree with this pattern, we can leave it like this.

ChrizZz110 · 2018-10-04T08:18:49Z

  /**
   * Returns the path to the vertex file containing only vertices with the specified label.
   *
   * @param label vertex label


It's the graphHead label not the vertex label. Same for description 2 lines above.

ChrizZz110 · 2018-10-04T08:23:46Z

+  /**
+   * {@inheritDoc}
+   *
+   * graph heads will be disposed at the moment. The following issue attempts to provide


(1) Upper case letter at the beginning of a sentence.
(2) Update the Class-JavaDoc with the additional graphs.csv file

ChrizZz110 · 2018-10-04T08:24:26Z

+  }
+
+  /**
+   * {@inheritDoc}


No @inheritdoc if no additional description is provided

ChrizZz110 · 2018-10-04T08:26:42Z

+import org.gradoop.flink.io.impl.csv.metadata.MetaData;
+
+/**
+ * /**


In the current online branch, it is not fixed.

ChrizZz110 · 2018-10-04T08:31:33Z

 * The string needs to be encoded in the following format:
 *
- * vertex-id;vertex-label;value_1|value_2|...|value_n
+ * {@code vertex-id;[graph-ids];vertex-label;value_1|value_2|...|value_n}


Line 25 -> Remove line with wrong comment definition

ChrizZz110 · 2018-10-04T08:43:10Z

+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class LogicalGraphCSVToCSVTest extends GradoopFlinkTestBase {


JavaDoc for class and functions missing

ChrizZz110 · 2018-10-04T08:44:23Z

+ */
+package org.gradoop.flink.io.impl.csv.conversion;
+
+  import org.gradoop.flink.io.api.DataSink;


Wrong indentation at import statements

ChrizZz110 · 2018-10-04T08:44:42Z

+  import org.junit.Test;
+  import org.junit.rules.TemporaryFolder;
+
+public class LogicalGraphIndexedCSVToCSVTest extends GradoopFlinkTestBase {


JavaDoc for class and functions missing

ChrizZz110 · 2018-10-04T08:45:13Z

+  }
+
+  @Test
+  public void testWriteLogicalGraph() throws Exception {


JavaDoc missing

ChrizZz110 · 2018-10-04T08:46:11Z

+  }
+
+  @Test
+  public void testReadSingleGraph() throws Exception {


JavaDoc missing

ChrizZz110

Perfect

…#976) fixes dbs-leipzig#551

This was referenced Sep 5, 2018

CSVDataSource fails when labels or properties contains separator characters #965

Closed

Change csv strings of complex property types (Map, List, Set) #985

Closed

p-f reviewed Sep 18, 2018

View reviewed changes

p-f suggested changes Sep 18, 2018

View reviewed changes

timo95 mentioned this pull request Sep 26, 2018

IndexedCSVDataSink fails to write default element labels #996

Closed

klaudworks and others added 4 commits September 27, 2018 09:17

[dbs-leipzig#551] csv graph collection import and export

19795cd

[dbs-leipzig#551] added converter from old to new csv formats

ea69962

[dbs-leipzig#551] minor cleanup re-positioning of deprecated package

302ea70

[dbs-leipzig#551] added conversion from old csv format to new csv for…

b75fd03

…mat and minor fixes

klaudworks force-pushed the #551_support_graph_collection branch from 019e261 to b75fd03 Compare September 27, 2018 12:31

[dbs-leipzig#551] [dbs-leipzig#985] Change CSV string representation …

f468b62

…for complex property types and clean indexed CSV paths of illegal characters

klaudworks added 2 commits September 28, 2018 11:23

[dbs-leipzig#551] referenced issue dbs-leipzig#974 to solve csv logic…

7c1640a

…al graph import

Merge branch '#551_support_graph_collection' of github.com:2start/gra…

c8b3d4c

…doop into #551_support_graph_collection

galpha requested changes Sep 28, 2018

View reviewed changes

[dbs-leipzig#551] comment fix

24faf72

galpha approved these changes Sep 28, 2018

View reviewed changes

p-f approved these changes Sep 28, 2018

View reviewed changes

galpha mentioned this pull request Sep 28, 2018

Prepare minor release 0.4.2 #1000

Closed

ChrizZz110 requested changes Oct 4, 2018

View reviewed changes

timo95 added 2 commits October 4, 2018 13:23

[dbs-leipzig#551] Fix JavaDoc

2021c2a

Merge branch 'develop' into #551_support_graph_collection

7eef5f0

ChrizZz110 approved these changes Oct 4, 2018

View reviewed changes

ChrizZz110 assigned galpha, p-f and ChrizZz110 Oct 4, 2018

ChrizZz110 merged commit bf4dac0 into dbs-leipzig:develop Oct 4, 2018

galpha pushed a commit to galpha/gradoop that referenced this pull request Oct 9, 2018

[dbs-leipzig#551] csv graph collection import and export (dbs-leipzig…

19590b4

…#976) fixes dbs-leipzig#551

0x002A pushed a commit to ChrizZz110/gradoop that referenced this pull request Feb 19, 2019

[dbs-leipzig#551] csv graph collection import and export (dbs-leipzig…

faa9e1a

…#976) fixes dbs-leipzig#551

Conversation

klaudworks commented Aug 31, 2018

Uh oh!

p-f left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p-f left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p-f commented Sep 26, 2018

Uh oh!

timo95 commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galpha commented Sep 28, 2018

Uh oh!

ChrizZz110 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrizZz110 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

p-f left a comment •

edited

Loading

timo95 commented Sep 27, 2018 •

edited

Loading