Add Flatten transform #3812

invokesus · 2018-05-25T15:53:29Z

TODO:

Use vega-typings
Docs
Example
Add docstrings in transform.ts

invokesus · 2018-05-25T15:57:32Z

src/compile/data/flatten.ts

+  private getDefaultName(field: string): string {
+    const index = this.transform.flatten.indexOf(field);
+    // Returns the "as" entry corresponding to field if it exists else returns field
+    return (this.transform.as === undefined || this.transform.as[index] === undefined) ? field : this.transform.as[index];


Right now this follows Vega Defaults. Not sure whether to change this and introduce different behavior when as is not specified.

Can't we just pass nothing and let Vega handle the default case?

@kanitw mentioned in slack that we need to set as, so that producedFields gives the correct output. But here, since the default is the field value, I think passing nothing might be ok.

Try it out and I'd always prefer to output less.

You can output as=undefined if you modify producedFields to output field name instead if as is undefined

I think this method is pretty unnecessary.

Try it out and I'd always prefer to output less.

I just check the rest of transform are all explicit though, so doing so would be quite inconsistent. So let's make them explicit for consistency for now.

If @domoritz doesn't like it, he can refactor all of the transforms later.

kanitw

Great start. Please see comments. :)

kanitw · 2018-05-25T20:55:51Z

src/compile/data/flatten.ts

+    const fields: string[] = [];
+    const as = [];
+    for (const field of this.transform.flatten) {
+      fields.push(field === undefined ? null : field);


why converting undefined to null?

kanitw · 2018-05-25T20:57:50Z

src/compile/data/flatten.ts

+  }
+
+  private getDefaultName(field: string): string {
+    const index = this.transform.flatten.indexOf(field);


The use of indexOf here is a bit unnecessary complex. Why don't you pass index in as a parameter. Your producedFields method can use the index in the reducer.

when referring to properties of an object multiple times, always use destructure syntax.

const {flatten, as} = this.transform

kanitw · 2018-05-25T21:00:35Z

src/compile/data/flatten.ts

+  }
+
+  public producedFields() {
+    const out = {};


Better just use reduce

return this.transform.flatten.reduce((index, field, i) => { out[as[i] || field] = true; return out }, {});

kanitw · 2018-05-25T21:03:45Z

src/compile/data/flatten.ts

+  private getDefaultName(field: string): string {
+    const index = this.transform.flatten.indexOf(field);
+    // Returns the "as" entry corresponding to field if it exists else returns field
+    return (this.transform.as === undefined || this.transform.as[index] === undefined) ? field : this.transform.as[index];


I think this method is pretty unnecessary.

kanitw · 2018-05-25T21:05:22Z

src/compile/data/flatten.ts

+  private getDefaultName(field: string): string {
+    const index = this.transform.flatten.indexOf(field);
+    // Returns the "as" entry corresponding to field if it exists else returns field
+    return (this.transform.as === undefined || this.transform.as[index] === undefined) ? field : this.transform.as[index];


Try it out and I'd always prefer to output less.

I just check the rest of transform are all explicit though, so doing so would be quite inconsistent. So let's make them explicit for consistency for now.

If @domoritz doesn't like it, he can refactor all of the transforms later.

kanitw

LGTM. Please make last minor fix and make the test pass and we can merge this to feature/transform. (Let's do docs separately so it's easier to review.)

kanitw · 2018-05-26T16:11:38Z

src/compile/data/flatten.ts

+    }, {});
+  }
+
+  private getNames() {


Remove this method and move the logic above?

producedFields and getNames

kanitw

Cool. We're almost done there. Here are some comments.

kanitw · 2018-05-29T21:09:52Z