[euphoria-core] #259 Hints are not runtime specific. #268

mareksimunek · 2018-02-16T15:22:49Z

#259 first part of unifying hints

implemented

public Dataset<OUT> output(Hint... hints)

Its breaking change because withHints on Join operator no longer exists

je-ik

I din't walk though the test, I think that they will need more tuning and adding more generic tests after the core part is done. Now I think that the most important change would be to move the hints from Dataset to operator.

je-ik · 2018-02-19T10:01:27Z

euphoria-core/src/main/java/cz/seznam/euphoria/core/client/dataset/Dataset.java


 /**
 * A dataset abstraction.
 *
 * @param <T> type of elements of this data set
 */
 @Audience(Audience.Type.CLIENT)
-public interface Dataset<T> extends Serializable {
+public interface Dataset<T> extends OutputHintAware<OutputHint>, Serializable {


I don't understand purpose of this interface. It looks odd in the context of client API. We should definitely keep this set on operator, not dataset, because operator is not (directly) part of client API.

interface removed. Hints are now on operator

je-ik · 2018-02-19T10:03:35Z

euphoria-core/src/main/java/cz/seznam/euphoria/core/client/dataset/Datasets.java

@@ -39,9 +42,9 @@
   * @return a dataset representing the output of the given operator
   */
  public static <IN, OUT> Dataset<OUT> createOutputFor(
-      Flow flow, Dataset<IN> input, Operator<IN, OUT> op) {
+      Flow flow, Dataset<IN> input, Operator<IN, OUT> op, Set<OutputHint> outputHints) {


If we set output hints on operator, there is no need for additional parameter here. On Beam branch, I already further simplified this method, because the parameter flow is redundant, because it can be retrieved from both input and op.

Hints are now on operator

je-ik · 2018-02-19T10:04:17Z

euphoria-core/src/main/java/cz/seznam/euphoria/core/client/dataset/InputDataset.java

 import javax.annotation.Nullable;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Set;

 /**
 * {@code PCollection} that is input of a {@code Flow}.


Although this is not part of this PR, can we fix the comment here?

fixed in #271

je-ik · 2018-02-19T10:05:23Z

euphoria-core/src/main/java/cz/seznam/euphoria/core/client/dataset/InputDataset.java

+  @Override
+  public Set<OutputHint> getHints() {
+    return Collections.emptySet();
+  }


Another +1 for moving hints to operator, this useless method will disappear. Moreover, the hints are called OutputHint, and Dataset has no "output". Operator does.

je-ik · 2018-02-19T10:07:53Z

euphoria-core/src/main/java/cz/seznam/euphoria/core/client/operator/Builders.java

@@ -106,24 +105,13 @@
     *
     * @return the dataset representing the new operator's output
     */
-    default Dataset<V> outputValues() {
+    default Dataset<V> outputValues(OutputHint... outputHints) {
      return MapElements
          .named("extract-values")
          .of(output())


The hints should be set here I think.

It shouldn't, output Dataset is with values. There would hint get lost. (last operator is MapElements). I added test.

Why would the hint get lost? The graph can be arbitrarily complicated and as a general rule, this should yeild the same runtime behavior:

Dataset input = ...; Dataset<Pair> output = ReduceByKey.of(input) ... .output(hints); MapElements.of(input) .using(Pair::getSecond) .output() .persist(...)

it should behave the same way as

Dataset input = ...; ReduceByKey.of(input) ... .outputValues(hints) .persist(...)

That is because non-shuffle operators should forward hints to downstream Datasets (unless overridden). That is due to the construction of the hints. The operator that the hint applies to is really RBK, not the MapElements.

After discussion, for now Hints doesn't propagate downstream in flow. Because we don't know if Hint will be still valid in next operator.

je-ik · 2018-02-19T10:11:07Z

euphoria-core/src/main/java/cz/seznam/euphoria/core/client/operator/FlatMap.java

      Flow flow = input.getFlow();
-      FlatMap<IN, OUT> map = new FlatMap<>(name, flow, input, functor, evtTimeFn);
+      FlatMap<IN, OUT> map = new FlatMap<>(name, flow, input, functor, evtTimeFn, Sets.newHashSet
+          (outputHints));


Opening parentheses placement.

je-ik · 2018-02-19T10:15:07Z

euphoria-core/src/main/java/cz/seznam/euphoria/core/client/operator/ReduceByKey.java

@@ -426,6 +443,11 @@ public boolean isCombinable() {
    return valueExtractor;
  }

+  @Override
+  public Dataset<Pair<KEY, OUT>> output(OutputHint... outputHints) {
+    return output();


This looks odd. Why two methods, when the varargs version can handle the zero argument version as well?

removed, because it cannot be reached (we are using Builders instead)

je-ik · 2018-02-19T10:16:13Z

euphoria-core/src/main/java/cz/seznam/euphoria/core/client/operator/hint/OutputHintAware.java

 import java.util.Set;

 @Audience(Audience.Type.INTERNAL)
-public interface HintAware<HINT extends Hint> {
+public interface OutputHintAware<HINT extends OutputHint> {


I would drop this whole interface.

je-ik · 2018-02-19T10:16:41Z

euphoria-core/src/main/java/cz/seznam/euphoria/core/client/operator/hint/SizeHint.java

+/**
+ * Extra information for runner about Dataset size
+ */
+public enum SizeHint implements OutputHint {


Please use @Audience annotation here.

je-ik · 2018-02-19T10:17:20Z

euphoria-core/src/test/java/cz/seznam/euphoria/core/client/operator/JoinTest.java

+    assertTrue(join.listInputs().stream().anyMatch(input -> ((Dataset) input).getHints().contains(new
+        TestHint())));
+    assertTrue(join.listInputs().stream().anyMatch(input -> ((Dataset) input).getHints().contains(new
+        TestHint2())));


These two lines wrap strangely.

…utputDataset

dmvk

LGTM, I have just few minor comments, good job 👍

dmvk · 2018-03-08T14:04:50Z

euphoria-core/src/main/java/cz/seznam/euphoria/core/client/operator/ReduceByKey.java

  }

+  @SuppressWarnings("unchecked")
+  ReduceByKey(String name,


i would prefer sticking with two constructors

dmvk · 2018-03-08T14:05:00Z

euphoria-core/src/main/java/cz/seznam/euphoria/core/client/operator/ReduceStateByKey.java

+        Collections.emptySet());
+  }
+
+  ReduceStateByKey(String name,


dmvk · 2018-03-08T14:10:27Z

euphoria-core/src/test/java/cz/seznam/euphoria/core/client/operator/JoinTest.java

+        ((Dataset) input).getProducer().getHints().contains(new Util.TestHint())));
+    assertTrue(join.listInputs().stream().anyMatch(input -> 
+        ((Dataset) input).getProducer().getHints().contains(new Util.TestHint2())));
+    assertEquals(2, ((Dataset) join.listInputs().stream().findFirst().get()).getProducer().getHints().size());


I reckon multilining this would be beneficial :)

dmvk · 2018-03-08T14:11:16Z

euphoria-core/src/main/java/cz/seznam/euphoria/core/client/operator/SumByKey.java

    this.valueExtractor = valueExtractor;
  }

  @Override
  public DAG<Operator<?, ?>> getBasicOps() {
    ReduceByKey<IN, KEY, Long, Long, W> reduceByKey =
        new ReduceByKey<>(getName(), input.getFlow(), input,
-        keyExtractor, valueExtractor, windowing, Sums.ofLongs());
+        keyExtractor, valueExtractor, windowing, Sums.ofLongs(), this.getHints());


we should stick with the same coding style :) Can we remove this.?

je-ik

LGTM

fixed tests [euphoria-core] hints in getBasicOps are only on last operator [euphoria-core] code style corrections [euphoria-core] code style corrections 2

[euphoria-core] #259 Hints are not runtime specific.

mareksimunek force-pushed the simunek/259/hintsUnified branch from 5443b02 to f16d343 Compare February 16, 2018 15:35

mareksimunek requested review from dmvk and je-ik February 16, 2018 16:09

je-ik suggested changes Feb 19, 2018

View reviewed changes

mareksimunek force-pushed the simunek/259/hintsUnified branch 2 times, most recently from 0e18e81 to 9482640 Compare March 7, 2018 13:12

[euphoria-core] #259 Hints are not runtime specific. Hints describe O…

1ddf75a

…utputDataset

mareksimunek force-pushed the simunek/259/hintsUnified branch from 9482640 to 6883329 Compare March 8, 2018 13:54

dmvk suggested changes Mar 8, 2018

View reviewed changes

je-ik approved these changes Mar 8, 2018

View reviewed changes

[euphoria-core] #259 Hints are now in operator, not Dataset

0fa9ff8

fixed tests [euphoria-core] hints in getBasicOps are only on last operator [euphoria-core] code style corrections [euphoria-core] code style corrections 2

mareksimunek force-pushed the simunek/259/hintsUnified branch from 43ae4b2 to 0fa9ff8 Compare March 8, 2018 14:33

dmvk approved these changes Mar 8, 2018

View reviewed changes

dmvk merged commit 6c3f3db into master Mar 8, 2018

dmvk deleted the simunek/259/hintsUnified branch March 8, 2018 15:23

mareksimunek pushed a commit that referenced this pull request May 4, 2018

Merge pull request #268 from seznam/simunek/259/hintsUnified

5a67abc

[euphoria-core] #259 Hints are not runtime specific.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[euphoria-core] #259 Hints are not runtime specific. #268

[euphoria-core] #259 Hints are not runtime specific. #268

mareksimunek commented Feb 16, 2018

je-ik left a comment

je-ik Feb 19, 2018

mareksimunek Mar 7, 2018

je-ik Feb 19, 2018

mareksimunek Mar 7, 2018

je-ik Feb 19, 2018

mareksimunek Mar 7, 2018

je-ik Feb 19, 2018

je-ik Feb 19, 2018

mareksimunek Mar 1, 2018

je-ik Mar 1, 2018

mareksimunek Mar 7, 2018

je-ik Feb 19, 2018

mareksimunek Mar 7, 2018

je-ik Feb 19, 2018

mareksimunek Mar 7, 2018

je-ik Feb 19, 2018

mareksimunek Mar 7, 2018

je-ik Feb 19, 2018

mareksimunek Mar 7, 2018

je-ik Feb 19, 2018

dmvk left a comment

dmvk Mar 8, 2018

dmvk Mar 8, 2018

dmvk Mar 8, 2018

dmvk Mar 8, 2018

je-ik left a comment

[euphoria-core] #259 Hints are not runtime specific. #268

[euphoria-core] #259 Hints are not runtime specific. #268

Conversation

mareksimunek commented Feb 16, 2018

je-ik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

je-ik left a comment

Choose a reason for hiding this comment