-
Notifications
You must be signed in to change notification settings - Fork 88
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
[#1004] create module, package structure and pom for data integration module #1015
Conversation
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.
Some small notes
import org.gradoop.flink.model.api.functions.TransformationFunction; | ||
|
||
/** | ||
* Each a given edge label each edge gets its label changed + source and target |
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.
Is there a typo? Or my English is not "the yellow from the egg" ^^ Maybe: An edge transformation that swaps the source and target of an edge with a given label and renames it.
public class InvertEdges implements TransformationFunction<Edge> { | ||
|
||
/** | ||
* label of the edges that should be inverted |
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.
Since we create and publish javadoc, please use sentences like The label of the edges that should be inverted.
private final String newLabel; | ||
|
||
/** | ||
* The constructor. |
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.
Constructs a new InvertEdges edge transformation function.
/** | ||
* The constructor. | ||
* | ||
* @param forEdgeLabel - the label if the edges that should be inverted |
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.
Typo ->if<- and the -
is not necessary because it will be inserted automatically at javadoc creation.
...p-data-integration/src/main/java/org/gradoop/dataintegration/transformation/InvertEdges.java
Show resolved
Hide resolved
*/ | ||
public class InvertEdgesTest extends GradoopFlinkTestBase { | ||
|
||
@Test(expected = NullPointerException.class) |
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.
Please provide a short javadoc at each test because we plan to enable checkstyle for test classes in the future.
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.
aha, g2k
} | ||
|
||
@Test | ||
public void testInvert() throws Exception { |
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.
I think a test for the inverting is missing. Only the renaming is tested. Note that every call to count()
is a own executed plan aik.
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.
Good point, I will add one.
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
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
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.
I'm a bit too late, sorry. But I think the test is broken.
List<Edge> newEdges = new ArrayList<>(); | ||
invertedEdgeGraph | ||
.getEdgesByLabel(invertedLabel) | ||
.output(new LocalCollectionOutputFormat<>(newEdges)); |
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.
There is a missing ExecutionEnvironment.execute()
after those, the vertices
and newEdges
collections will both be empty.
*/ | ||
List<Vertex> vertices = new ArrayList<>(); | ||
invertedEdgeGraph.getVertices() | ||
.filter(new And<>(new ByLabel<>("Person"), new ByLabel<>("Tag"))) |
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.
This should be an Or<>
instead, this filter will accept no vertices.
String sourceName = idMap.get(e.getSourceId()); | ||
String targetName = idMap.get(e.getTargetId()); | ||
|
||
Assert.assertTrue("source: " + sourceName + " | target: " + targetName, |
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.
This assert will always fail (since the idMap
is empty and Collection.contains(null)
is always false), but is never actually reached, since newEdges
is empty.
Oh - you are absolutely right. @merando has to provide a Bug ticket and PR for that. If three people have not seen that, then the fourth must be smarter ^^ |
…integration module (dbs-leipzig#1015) * fixes [dbs-leipzig#1004] created module, package structure and pom * fixes [dbs-leipzig#1009] added invert edges structural transformation
No description provided.