Merge identical transforms at forks #4029

invokesus · 2018-07-16T16:16:52Z

No description provided.

invokesus · 2018-07-26T21:40:42Z

Fixes #2177 and one of the problems @kanitw mentioned in #3797.

[
  {"data": "data_0", "field": "Origin"},
  {"data": "data_1", "field": "Origin"}
]

now is

{"data": "source_0", "field": "mean_Miles_per_Gallon"}

kanitw · 2018-07-27T00:26:51Z

examples/compiled/concat_layer_voyager_result_future.vg.json

@@ -70,16 +70,6 @@
      "name": "data_3",
      "source": "data_1",
      "transform": [{"type": "filter", "expr": "datum.label !== 'PoleStar'"}]
-    },


In the future PRs, we should try to see how we can make data_2 and data_3 merge for this example. (No need for this PR.)

kanitw · 2018-07-27T00:31:14Z

src/compile/data/filter.ts

@@ -22,4 +22,9 @@ export class FilterNode extends DataFlowNode {
      expr: this.expr
    };
  }
+
+  public hash() {
+    // FIX include something from model in hash


Hmm I thought we agreed to decouple SelectionFilter from ExpressionFilter.
So we only merge ExpressionFilter, not SelectionFilter.

That said, the current code means that it will always be merged. The question is whether this is the right behavior. Maybe it's fine.

To test it, create an spec where the same selection is applied two layers and see if merging the results still produce correct output. (Maybe concat one unit spec with a layered spec then apply selection from the unit spec to both layers in the other spec.)

Actually, modify interactive_stocks_nearest_index to use timeUnit would demonstrate if this works well.

The sad news is that adding timeUnit to interactive_stocks_nearest_index doesn't even work, but the diff that this PR is causing to that version seems reasonable.

Thus, I wonder if we should ignore this case for now.

@domoritz any thoughts?

In any case, @invokesus we better remove // FIX include something from model in hash. I don't think we should add stuff from the model to the hash, but rather get rid of model from Filter #4084

kanitw · 2018-07-27T00:32:48Z

src/compile/data/optimize.ts

@@ -161,6 +161,7 @@ export function optimizeDataflow(dataComponent: DataComponent) {

  roots.forEach(moveFacetDown);
  roots.forEach(mergeParse);
+  roots.forEach(optimizers.mergeIdenticalTransforms);


Hmm, why is the call pattern different from others (mergeParse isn't in optimizers?)

cc: @domoritz

mergeParse should be in optimizers. It's not super clear because in a way it's necessary for correctness as well.

kanitw · 2018-07-27T03:22:16Z

LGTM, but I'll let @domoritz approve.

invokesus · 2018-07-27T15:18:43Z

The coverage test is failing but I don't think it makes sense to write new tests for the concerned files.

domoritz · 2018-07-27T15:45:26Z

src/compile/data/optimizers.ts

+  const hashes = transforms.map(x => x.hash());
+  const buckets = {};
+  for (let i = 0; i < hashes.length; i++) {
+    if (buckets[hashes[i]] === undefined) {


buckets[hashes[i]] = (buckets[hashes[i]] || []).push(transforms[i])?

push doesn't return the array only the element pushed.

I guess we could use concat but whatever.

domoritz · 2018-07-27T15:48:41Z

src/compile/data/optimize.ts

@@ -161,6 +161,7 @@ export function optimizeDataflow(dataComponent: DataComponent) {

  roots.forEach(moveFacetDown);
  roots.forEach(mergeParse);
+  roots.forEach(optimizers.mergeIdenticalTransforms);


Wouldn't it be better to start from the leaves?

Yeah I think so. But I thought it would be better to have the top-down version working with the mergeParse(which is also top-down) and refactor them both to bottom-up when I've got the multiple pass thing working.

Okay, just don't forget

domoritz · 2018-07-27T15:50:57Z

src/compile/data/bin.ts

@@ -144,6 +144,9 @@ export class BinNode extends DataFlowNode {
    return out;
  }

+  public hash() {
+    return hash(this.bins);


Could there be problems since the node type is not part of the hash?

Good point, I'll include it in the hash.
What's the best way though? Adding a nodeType key?

Maybe something like
return hash({nodeType: "BinNode", ...this.bins});

Or just type + hash

domoritz · 2018-07-27T15:51:47Z

src/compile/data/window.ts

@@ -59,6 +59,10 @@ export class WindowTransformNode extends DataFlowNode {
    return windowFieldDef.as || vgField(windowFieldDef);
  }

+  public hash() {
+    return hash(this.transform);


Do you think it makes sense to have tests for these hash functions so we can read them in the diff and see whether they look good?

Are you suggesting tests like
i) hash(node) === hash(node.clone())
or
ii) hash(node) === '__hash_value__'
I see the benefit that test ii might provide, but the value will have to be updated each time we change things.
However, I think I should write some tests for the hash function itself with probable inputs in util.test.ts.

domoritz · 2018-07-30T17:22:26Z

src/compile/data/stack.ts

@@ -191,6 +192,10 @@ export class StackNode extends DataFlowNode {
    }, {});
  }

+  public hash() {
+    return 'StackNode' + hash(this._stack);


Stack ${hash(this._stack)}

I don't care but @kanitw suggested that we already use colons in the hash. It doesn't really matter so I'd say space is fine.

It looks nicer in the test without colon lol -- not that it's a big deal.

invokesus · 2018-08-01T22:52:00Z

Let me know if I missed anything.

domoritz · 2018-08-01T23:06:25Z

Remove "Node" from hash

domoritz · 2018-08-02T16:55:24Z

🎉

invokesus added the 🚧 WIP label Jul 16, 2018

domoritz changed the title ~~[WIP] Merge identical transforms at forks~~ Merge identical transforms at forks Jul 16, 2018

invokesus force-pushed the inv/merge-identical-transforms branch 2 times, most recently from 546be4c to bb4def7 Compare July 18, 2018 19:24

invokesus force-pushed the inv/merge-identical-transforms branch 3 times, most recently from d2e2cde to 201f48b Compare July 26, 2018 21:03

invokesus removed the 🚧 WIP label Jul 26, 2018

kanitw reviewed Jul 27, 2018

View reviewed changes

invokesus force-pushed the inv/merge-identical-transforms branch from 97e3ac4 to 0b2f712 Compare July 27, 2018 10:00

invokesus force-pushed the inv/merge-identical-transforms branch from 607f032 to 0b2f712 Compare July 27, 2018 15:28

domoritz reviewed Jul 27, 2018

View reviewed changes

invokesus force-pushed the inv/merge-identical-transforms branch 2 times, most recently from 0882e01 to e6e6f03 Compare July 28, 2018 07:38

domoritz reviewed Jul 30, 2018

View reviewed changes

invokesus force-pushed the inv/merge-identical-transforms branch from ff19493 to d16240b Compare August 1, 2018 22:08

invokesus added 7 commits August 2, 2018 03:40

Merge identical transforms at forks

dcd7cbd

Modify failing test

f4371ab

Create TransformNode abstract class and extend it

c5b474d

Clean up

d08c5ad

Remove comment

beceade

Add node type to hash

47b5d4a

Tests for hash methods

153765e

invokesus force-pushed the inv/merge-identical-transforms branch from d16240b to ea53045 Compare August 1, 2018 22:15

Add space to hash

ea53045

invokesus force-pushed the inv/merge-identical-transforms branch from 07632f9 to e7ebd7a Compare August 1, 2018 22:32

Fix rebase errors

e7ebd7a

invokesus force-pushed the inv/merge-identical-transforms branch from 608f261 to eca0d12 Compare August 1, 2018 23:19

invokesus and others added 2 commits August 2, 2018 04:50

Remove "Node" from hash

eca0d12

[Travis] Update examples (build: 18764)

6036c62

domoritz merged commit 1dad6bf into master Aug 2, 2018

domoritz deleted the inv/merge-identical-transforms branch August 2, 2018 16:55

domoritz mentioned this pull request Sep 26, 2018

boxplot doesn't works with column encoding #4156

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge identical transforms at forks #4029

Merge identical transforms at forks #4029

invokesus commented Jul 16, 2018

invokesus commented Jul 26, 2018

kanitw Jul 27, 2018

kanitw Jul 27, 2018

kanitw Jul 27, 2018

kanitw Jul 27, 2018

kanitw Jul 27, 2018

domoritz Jul 27, 2018

kanitw commented Jul 27, 2018

invokesus commented Jul 27, 2018

domoritz Jul 27, 2018

invokesus Jul 28, 2018

domoritz Aug 2, 2018

domoritz Jul 27, 2018

invokesus Jul 27, 2018

domoritz Jul 27, 2018

domoritz Jul 27, 2018

invokesus Jul 27, 2018

domoritz Jul 27, 2018

domoritz Jul 27, 2018

invokesus Jul 27, 2018

domoritz Jul 27, 2018

domoritz Jul 30, 2018 •

edited by invokesus

invokesus Jul 30, 2018

domoritz Jul 30, 2018

kanitw Jul 30, 2018

invokesus commented Aug 1, 2018

domoritz commented Aug 1, 2018

domoritz commented Aug 2, 2018

Merge identical transforms at forks #4029

Merge identical transforms at forks #4029

Conversation

invokesus commented Jul 16, 2018

invokesus commented Jul 26, 2018

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

kanitw commented Jul 27, 2018

invokesus commented Jul 27, 2018

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

domoritz Jul 30, 2018 • edited by invokesus

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

invokesus commented Aug 1, 2018

domoritz commented Aug 1, 2018

domoritz commented Aug 2, 2018

domoritz Jul 30, 2018 •

edited by invokesus