Correctly generate `as` for nested fields. Note that in Vega `as` cannot be nested so you have to provide a field name, not a field accessor. Fixes #3744 #4023

kanitw · 2018-07-15T03:48:56Z

No description provided.

…not be nested so you have to provide a field name, not a field accessor. Fixes #3744.

kanitw

Lucky we don't let it go.

These two names are super confusing: vgFieldName vgField. Both are field names. If one of them is transform output, better make that a parameter.

domoritz · 2018-07-15T04:19:09Z

Updated the PR.

kanitw

LGTM with a few minor knitpicks.

kanitw · 2018-07-15T04:31:52Z

src/compile/data/stack.ts

@@ -218,6 +218,7 @@ export class StackNode extends DataFlowNode {
    // Impute
    if (impute && dimensionFieldDef) {
      const dimensionField = dimensionFieldDef ? vgField(dimensionFieldDef, {binSuffix: 'mid'}) : undefined;
+      const dimensionAs = dimensionFieldDef ? vgField(dimensionFieldDef, {binSuffix: 'mid', forAs: true}) : undefined;


Why should this be different from dimensionField?
especially given than you impute afterward.

The key is a field reference in Vega and we have to escape nesting.

I revides the code a bit since it's a bit superfluous to have a ternary with dimensionFieldDef if it is already required to pass the if.

kanitw · 2018-07-15T04:33:17Z

src/compile/facet.ts

+export function facetSortFieldName(
+  fieldDef: FacetFieldDef<string>,
+  sort: EncodingSortField<string>,
+  expr?: 'datum',


Make {expr, forAs=true}: {expr?: datum, forAs:boolean}?

No, we want the default to be false.

Anyway, revised.

kanitw · 2018-07-15T04:38:09Z

src/fielddef.ts

+  if (opt.forAs) {
+    // Don't need to mess with nested field names because Vega transform outputs are never nested.
+    return field;
+  }


kanitw · 2018-07-15T04:40:20Z

src/util.ts

@@ -297,7 +297,8 @@ export function accessPathWithDatum(path: string, datum = 'datum') {
 }

 /**
- * Return access with datum to the falttened field.
+ * Return access with datum to the flattened field.


FYI, the datum variable down there is a bit misleading as it can be parent too. (It confuses me for a bit)

kanitw · 2018-07-15T04:41:55Z

src/fielddef.ts

  suffix?: string;
+  /** Use the field name for `as` in a transform. */


Add a comment how this affect the output.

kanitw · 2018-07-15T04:42:23Z

src/fielddef.ts

@@ -396,6 +401,11 @@ export function vgField(
    field = `${prefix}_${field}`;
  }

+  if (opt.forAs) {
+    // Don't need to mess with nested field names because Vega transform outputs are never nested.


"mess with" => escape

Correctly generate as for nested fields. Note that in Vega as can…

3052d7b

…not be nested so you have to provide a field name, not a field accessor. Fixes #3744.

kanitw changed the title ~~Dom/nested aggregates~~ Correctly generate as for nested fields. Note that in Vega as cannot be nested so you have to provide a field name, not a field accessor. Fixes #3744 Jul 15, 2018

kanitw commented Jul 15, 2018

View reviewed changes

domoritz and others added 2 commits July 14, 2018 21:18

Replace vgFieldName with just vgField but add opt parameter

0ddd4a9

[Travis] Update examples (build: 18035)

4a684d2

domoritz force-pushed the dom/nested-aggregates branch from 3501438 to 4a684d2 Compare July 15, 2018 04:18

kanitw commented Jul 15, 2018

View reviewed changes

Revisions

bc77b1e

domoritz force-pushed the dom/nested-aggregates branch from 7535c01 to bc77b1e Compare July 15, 2018 05:23

kanitw merged commit 2085ac0 into master Jul 15, 2018

kanitw deleted the dom/nested-aggregates branch July 15, 2018 05:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly generate `as` for nested fields. Note that in Vega `as` cannot be nested so you have to provide a field name, not a field accessor. Fixes #3744 #4023

Correctly generate `as` for nested fields. Note that in Vega `as` cannot be nested so you have to provide a field name, not a field accessor. Fixes #3744 #4023

kanitw commented Jul 15, 2018

kanitw left a comment

domoritz commented Jul 15, 2018

kanitw left a comment

kanitw Jul 15, 2018

domoritz Jul 15, 2018

domoritz Jul 15, 2018

kanitw Jul 15, 2018

domoritz Jul 15, 2018

domoritz Jul 15, 2018

kanitw Jul 15, 2018

domoritz Jul 15, 2018

kanitw Jul 15, 2018

domoritz Jul 15, 2018

kanitw Jul 15, 2018

domoritz Jul 15, 2018

kanitw Jul 15, 2018

domoritz Jul 15, 2018

		suffix?: string;
		/** Use the field name for `as` in a transform. */

Correctly generate as for nested fields. Note that in Vega as cannot be nested so you have to provide a field name, not a field accessor. Fixes #3744 #4023

Correctly generate as for nested fields. Note that in Vega as cannot be nested so you have to provide a field name, not a field accessor. Fixes #3744 #4023

Conversation

kanitw commented Jul 15, 2018

kanitw left a comment

Choose a reason for hiding this comment

domoritz commented Jul 15, 2018

kanitw 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

Correctly generate `as` for nested fields. Note that in Vega `as` cannot be nested so you have to provide a field name, not a field accessor. Fixes #3744 #4023

Correctly generate `as` for nested fields. Note that in Vega `as` cannot be nested so you have to provide a field name, not a field accessor. Fixes #3744 #4023