Skip to content
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

[#936] Generalized RollUp and DrillDown #939

Merged

Conversation

0x002A
Copy link
Contributor

@0x002A 0x002A commented Aug 12, 2018

No description provided.

@0x002A 0x002A changed the title [#936] Current Version [#936] Generalized DrillUp and DrillDown Aug 12, 2018
@0x002A 0x002A force-pushed the #936_generalize_drillup_and_drilldown branch from 4a059f5 to 8d07ebc Compare August 13, 2018 07:43
@@ -575,13 +558,11 @@ public void testVertexDrillDownAfterDrillUpNewPropertyKey() throws Exception {
.callForGraph(new Drill.DrillBuilder()
.setPropertyKey("memberCount")
.setNewPropertyKey("memberCount_in_K")
.setFunction(new DrillDivideBy(1000L))
.drillVertex(true)
.setVertexDrillFunction(new DrillDivideBy(1000L))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for GraphHeadDrillFunction is missing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Test containing Builder.setEdgeDrillFunction() (enhance coverage)

/**
* Supported elements.
*/
public static enum Element {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inner Enum doesn't need to be static (redundant)

* @param function drill function which shall be applied to a property
* @param newPropertyKey new property key
* @param drillVertex true, if vertices shall be drilled, false for edges
* @param label label of the element whose property shall be drilled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace between Javadoc comment and param describtion

   * Valued constructor.
   * 
   * @param label           label of the element whose property shall be drilled

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • javadoc style
   * @param label                   label of the element whose property shall be drilled
   * @param propertyKey             property key
   * @param vertexDrillFunction     drill function which shall be applied to a property of a vertex
   * @param edgeDrillFunction       drill function which shall be applied to a property of an edge
   * @param graphheadDrillFunction  drill function which shall be applied to a property of a
   *                                graph head
   * @param newPropertyKey          new property key
   * @param element                 Element to be covered by the operation

@@ -232,7 +316,8 @@ public DrillUp buildDrillUp() {
public DrillDown buildDrillDown() {
Objects.requireNonNull(propertyKey);
return new DrillDown(
label, propertyKey, function, newPropertyKey, drillVertex);
label, propertyKey, vertexDrillFunction, edgeDrillFunction, graphheadDrillFunction,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IllegalArgumentException (in case of no functions given) should be thrown here as well.

Copy link
Contributor Author

@0x002A 0x002A Aug 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A DrillDown without a specific DrillFunction is valid in case of a preceded DrillUp.

graph = graph.transformVertices(
new DrillDownTransformation<Vertex>(getLabel(), getPropertyKey(), getFunction(),
new DrillDownTransformation<Vertex>(getLabel(), getPropertyKey(), getVertexDrillFunction(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be empty diamonds <>. Below as well.

DrillUp(String label, String propertyKey, DrillFunction function, String newPropertyKey,
boolean drillVertex) {
super(label, propertyKey, function, newPropertyKey, drillVertex);
DrillUp(String label, String propertyKey, DrillFunction vertexDrillFunction,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess a rename to RollUp makes sense here.

graph = graph.transformVertices(
new DrillUpTransformation<Vertex>(getLabel(), getPropertyKey(), getFunction(),
new DrillUpTransformation<Vertex>(getLabel(), getPropertyKey(), getVertexDrillFunction(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be empty diamonds <> as well.

@galpha galpha self-assigned this Aug 13, 2018
@galpha galpha changed the title [#936] Generalized DrillUp and DrillDown [#936] Generalized RollUp and DrillDown Aug 15, 2018
@@ -438,24 +438,24 @@ public LogicalGraph reduceOnNeighbors(
* {@inheritDoc}
*/
@Override
public LogicalGraph drillUpVertex(String propertyKey, DrillFunction function) {
return drillUpVertex(null, propertyKey, function);
public LogicalGraph rollUpVertex(String propertyKey, DrillFunction function) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add rollUp and drillDown functions for graphhead as well.

@@ -56,15 +53,15 @@ public DrillDown(
public LogicalGraph execute(LogicalGraph graph) {
if (getElement() == Element.VERTICES) {
graph = graph.transformVertices(
new DrillDownTransformation<Vertex>(getLabel(), getPropertyKey(), getVertexDrillFunction(),
new DrillDownTransformation<>(getLabel(), getPropertyKey(), getVertexDrillFunction(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for readability i prefer a switch case here

    switch (getElement()) {
    case VERTICES:
      graph = graph.transformVertices(
        new DrillDownTransformation<>(
          getLabel(),
          getPropertyKey(),
          getVertexDrillFunction(),
          getNewPropertyKey(),
          drillAllLabels(),
          keepCurrentPropertyKey()));
      break;
    case EDGES:
      graph = graph.transformEdges(
        new DrillDownTransformation<>(
          getLabel(),
          getPropertyKey(),
          getEdgeDrillFunction(),
          getNewPropertyKey(),
          drillAllLabels(),
          keepCurrentPropertyKey()));
      break;
    case GRAPHHEAD:
      graph = graph.transformGraphHead(
        new DrillDownTransformation<>(
          getLabel(),
          getPropertyKey(),
          getGraphheadDrillFunction(),
          getNewPropertyKey(),
          drillAllLabels(),
          keepCurrentPropertyKey()));
      break;
      default: throw new UnsupportedTypeException("Element type must be vertex, edge or graphhead");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -71,15 +68,15 @@
public LogicalGraph execute(LogicalGraph graph) {
if (getElement() == Element.VERTICES) {
graph = graph.transformVertices(
new DrillUpTransformation<Vertex>(getLabel(), getPropertyKey(), getVertexDrillFunction(),
new RollUpTransformation<>(getLabel(), getPropertyKey(), getVertexDrillFunction(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above


import java.util.List;

import org.apache.flink.api.java.io.LocalCollectionOutputFormat;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of unused imports here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

//----------------------------------------------------------------------------


@Test(expected = NullPointerException.class)
public void testVertexDrillUpNoProperty() {
public void testVertexRollUpNoProperty() {
FlinkAsciiGraphLoader loader = getLoaderFromString(getDrillInput());

LogicalGraph input = loader.getLogicalGraphByVariable("input");

LogicalGraph output = input.callForGraph(new Drill.DrillBuilder()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

output is unused

Copy link
Contributor Author

@0x002A 0x002A Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unit tests checks if a specific exception is thrown in case of missing parameters. So it's okay if there is no assertion in place.

FlinkAsciiGraphLoader loader = getLoaderFromString(getDrillInput());

LogicalGraph input = loader.getLogicalGraphByVariable("input");

LogicalGraph output = input
.callForGraph(new Drill.DrillBuilder()
.setVertexDrillFunction(new DrillDivideBy(1000L))
.buildDrillUp());
.buildRollUp());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this chain is never executed -> no assertion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

FlinkAsciiGraphLoader loader = getLoaderFromString(getDrillInput());
LogicalGraph input = loader.getLogicalGraphByVariable("input");

loader.appendToDatabaseFromString("expected{title : \"Graph\", globalMemberCount : 42L,globalMemberCount__1: 42000L}[" +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"expected" on new line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@galpha galpha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@galpha galpha merged commit 8ba0ced into dbs-leipzig:develop Aug 15, 2018
ChrizZz110 pushed a commit that referenced this pull request Aug 24, 2018
### Release 0.4.1

* [#877] added support for property value null (#908)

fixes #877

* [#856] property pushdown hbase (#906)

fixes #856

* [#864] Change examples to use csv (#905)

fixes #864

* [#494] Add Set support to PropertyValue (#916)

fixes #494

* [#888] Minor changes to Tuple-related functions. (#912)

fixes #888

* [#779] Add code coverage for Gradoop (#914)

fixes #779

* [#702] Add support for graphs without edges to TLFDataSource (#921)

fixes #702

* [#909] Remove Flink dependency for store instance (#918)

fixes #909

* [#876] added print for logical graph and graph collection (#913)

fixes #876

* [#857] logical predicate chaining hbase (#920)

fixes #857

* [#926] added argLine to maven properties (fix ide testing bug) (#929)

fixes #926

* [#794] include new benchmark classes (#904)

fixes #794

* [#312] example program for aggregation (#925)

fixes #312

* [#901] Add SamplingConstants, AggregationConstants (#934)

fixed #901

* [#935] Add tests for containment functions (#941)

fixes #935

* [#900] Refactor sampling tests. (#907)

fixes #900

* [#936] Generalized RollUp and DrillDown (#939)

fixes #936

* [#947] Fix PageRank wrapper and add a test. (#949)

fixes #947

* [#942] graph density computation for sampling evaluation (#946)

fixes #942

* [#954] prepare minor release (#955)

fixes #954
0x002A added a commit to ChrizZz110/gradoop that referenced this pull request Feb 19, 2019
0x002A pushed a commit to ChrizZz110/gradoop that referenced this pull request Feb 19, 2019
### Release 0.4.1

* [dbs-leipzig#877] added support for property value null (dbs-leipzig#908)

fixes dbs-leipzig#877

* [dbs-leipzig#856] property pushdown hbase (dbs-leipzig#906)

fixes dbs-leipzig#856

* [dbs-leipzig#864] Change examples to use csv (dbs-leipzig#905)

fixes dbs-leipzig#864

* [dbs-leipzig#494] Add Set support to PropertyValue (dbs-leipzig#916)

fixes dbs-leipzig#494

* [dbs-leipzig#888] Minor changes to Tuple-related functions. (dbs-leipzig#912)

fixes dbs-leipzig#888

* [dbs-leipzig#779] Add code coverage for Gradoop (dbs-leipzig#914)

fixes dbs-leipzig#779

* [dbs-leipzig#702] Add support for graphs without edges to TLFDataSource (dbs-leipzig#921)

fixes dbs-leipzig#702

* [dbs-leipzig#909] Remove Flink dependency for store instance (dbs-leipzig#918)

fixes dbs-leipzig#909

* [dbs-leipzig#876] added print for logical graph and graph collection (dbs-leipzig#913)

fixes dbs-leipzig#876

* [dbs-leipzig#857] logical predicate chaining hbase (dbs-leipzig#920)

fixes dbs-leipzig#857

* [dbs-leipzig#926] added argLine to maven properties (fix ide testing bug) (dbs-leipzig#929)

fixes dbs-leipzig#926

* [dbs-leipzig#794] include new benchmark classes (dbs-leipzig#904)

fixes dbs-leipzig#794

* [dbs-leipzig#312] example program for aggregation (dbs-leipzig#925)

fixes dbs-leipzig#312

* [dbs-leipzig#901] Add SamplingConstants, AggregationConstants (dbs-leipzig#934)

fixed dbs-leipzig#901

* [dbs-leipzig#935] Add tests for containment functions (dbs-leipzig#941)

fixes dbs-leipzig#935

* [dbs-leipzig#900] Refactor sampling tests. (dbs-leipzig#907)

fixes dbs-leipzig#900

* [dbs-leipzig#936] Generalized RollUp and DrillDown (dbs-leipzig#939)

fixes dbs-leipzig#936

* [dbs-leipzig#947] Fix PageRank wrapper and add a test. (dbs-leipzig#949)

fixes dbs-leipzig#947

* [dbs-leipzig#942] graph density computation for sampling evaluation (dbs-leipzig#946)

fixes dbs-leipzig#942

* [dbs-leipzig#954] prepare minor release (dbs-leipzig#955)

fixes dbs-leipzig#954
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants