fix: preserve node order in swapWithParent

[RobinL]

This PR closes #4680 and #5475. It helps with part of #5261.

The root cause of the problem is within the moveFacetDown map.

The problem occurs because the swapWithParent method does not preserve the order of branches in the tree, so the main map iterates incorrectly. It expects to be able to iterate over each branch in turn, but when the order changes, this fails.

I have used the following spec to find the bug.

Example buggy spec

{
  "$schema": "https://vega.github.io/schema/vega-lite/v5.json",
  "data": {
    "values": [
      { "metric_1": 10, "metric_2": 1, "y_value": 0.2, "row_group": false },
      { "metric_1": 11, "metric_2": 1, "y_value": 0.3, "row_group": false },
      { "metric_1": 12, "metric_2": 2, "y_value": 0.1, "row_group": false },
      { "metric_1": 13, "metric_2": 2, "y_value": 0.8, "row_group": false },
      { "metric_1": 12, "metric_2": 2, "y_value": 0.7, "row_group": true },
      { "metric_1": 13, "metric_2": 3, "y_value": 0.2, "row_group": true },
      { "metric_1": 14, "metric_2": 3, "y_value": 0.4, "row_group": true },
      { "metric_1": 15, "metric_2": 4, "y_value": 0.9, "row_group": true } ]
  },
  "hconcat": [
    {
      "encoding": {
        "row": {
          "field": "row_group",
          "type": "nominal"
        },
        "x": {
          "field": "metric_2",
          "type": "quantitative"
        },
        "y": {
          "field": "y_value",
          "title": null,
          "type": "quantitative"
        }
      },
      "mark": "bar"
    },
    {
      "encoding": {
        "row": {
          "field": "row_group",
          "type": "nominal"
        },
        "x": {
          "field": "metric_1",
          "type": "quantitative"
        },
        "y": {
          "field": "y_value",
          "title": null,
          "type": "quantitative"
        }
      },
      "mark": "bar"
    }
  ]
}

To be more specific, with this example spec, in optimize.ts immediately prior to the moveFacetDown map, data.sources is a symmetrical tree like:

Source
├── OutputNode
│   └── FacetNode
│       └── FilterInvalidNode
│           └── OutputNode
└── OutputNode
    └── FacetNode
        └── FilterInvalidNode
            └── OutputNode

However, following the map, the data structure has become asymmetrical. This is because swapWithParent does not preserve the order of the children. But we're iterating recursively over these children, so the recursive map becomes asymetrical.

The order fails to be preserved because when you remove a child and then re-add it here, its position in the array is not preserved.

I have tested this against the following specs which are reported as broken in #4680, with the following outputs, which look correct to me.

Grateful for any feedback on the fix. I've run tests locally, but I'm not sure I understand [this] about adding a GH_PAT. Is that saying that, in my fork, I need to add a secret containing a personal access token that has only public_repo permissions? I didn't want to do that without checking, because of potential security implications.

Specs:

My example used to bugfix

{
  "$schema": "https://vega.github.io/schema/vega-lite/v5.json",
  "data": {
    "values": [
      { "metric_1": 10, "metric_2": 1, "y_value": 0.2, "row_group": false },
      { "metric_1": 11, "metric_2": 1, "y_value": 0.3, "row_group": false },
      { "metric_1": 12, "metric_2": 2, "y_value": 0.1, "row_group": false },
      { "metric_1": 13, "metric_2": 2, "y_value": 0.8, "row_group": false },
      { "metric_1": 12, "metric_2": 2, "y_value": 0.7, "row_group": true },
      { "metric_1": 13, "metric_2": 3, "y_value": 0.2, "row_group": true },
      { "metric_1": 14, "metric_2": 3, "y_value": 0.4, "row_group": true },
      { "metric_1": 15, "metric_2": 4, "y_value": 0.9, "row_group": true } ]
  },
  "hconcat": [
    {
      "encoding": {
        "row": {
          "field": "row_group",
          "type": "nominal"
        },
        "x": {
          "field": "metric_2",
          "type": "quantitative"
        },
        "y": {
          "field": "y_value",
          "title": null,
          "type": "quantitative"
        }
      },
      "mark": "bar"
    },
    {
      "encoding": {
        "row": {
          "field": "row_group",
          "type": "nominal"
        },
        "x": {
          "field": "metric_1",
          "type": "quantitative"
        },
        "y": {
          "field": "y_value",
          "title": null,
          "type": "quantitative"
        }
      },
      "mark": "bar"
    }
  ]
}

cpennington example

{
  "$schema": "https://vega.github.io/schema/vega-lite/v3.json",
  "data": {
    "values": [
      {
        "c1": "Argagarg",
        "c2": "Argagarg",
        "mu": "2-8",
        "win_chance": 0.2,
        "p": 0.005293199778439004,
        "type": "global",
        "cum_p": 0.49384000294641295
      },
      {
        "c1": "Argagarg",
        "c2": "BBB",
        "mu": "2.5-7.5",
        "win_chance": 0.25,
        "p": 0.006317783956548739,
        "type": "global",
        "cum_p": 0.4182141361083573
      },
      {
        "c1": "BBB",
        "c2": "Argagarg",
        "mu": "4-6",
        "win_chance": 0.39999999999999997,
        "p": 0.00599613286508726,
        "type": "global",
        "cum_p": 0.5810908430205882
      },
      {
        "c1": "BBB",
        "c2": "BBB",
        "mu": "2.5-7.5",
        "win_chance": 0.25,
        "p": 0.017437015257325033,
        "type": "global",
        "cum_p": 0.5011573865688447
      }
    ]
  },
  "transform": [
    {
      "joinaggregate": [
        {
          "op": "mean",
          "field": "cum_p",
          "as": "c1_avg_cum_p"
        }
      ],
      "groupby": [
        "c1"
      ]
    }
  ],
  "hconcat": [
    {
      "facet": {
        "row": {
          "field": "c1",
          "type": "ordinal"
        },
        "column": {
          "field": "c2",
          "type": "ordinal"
        }
      },
      "spec": {
        "mark": {
          "type": "bar"
        },
        "encoding": {
          "x": {
            "field": "mu",
            "type": "ordinal"
          },
          "y": {
            "field": "p",
            "type": "quantitative"
          },
          "color": {
            "field": "cum_p",
            "type": "quantitative"
          }
        }
      }
    },
    {
      "facet": {
        "row": {
          "field": "c1",
          "type": "ordinal"
        }
      },
      "spec": {
        "mark": {
          "type": "bar"
        },
        "encoding": {
          "x": {
            "field": "mu",
            "type": "ordinal"
          },
          "y": {
            "aggregate": "mean",
            "field": "p",
            "type": "quantitative"
          },
          "color": {
            "aggregate": "mean",
            "field": "c1_avg_cum_p",
            "type": "quantitative"
          }
        }
      }
    }
  ]
}

sh4pe example

{
  "$schema": "https://vega.github.io/schema/vega-lite/v4.json",
  "data": {
    "values": [
      {
        "a": 0.1,
        "x": -5,
        "b": 0.1,
        "date": "2019-12-01T00:00:00"
      },
      {
        "a": 0.1,
        "x": 5,
        "b": 0.2,
        "date": "2019-12-01T00:00:00"
      },
      {
        "a": 0.1,
        "x": -55,
        "b": 0.1,
        "date": "2019-12-03T00:00:00"
      },
      {
        "a": 0,
        "x": -45,
        "b": 0.1,
        "date": "2019-12-03T00:00:00"
      }
    ]
  },
  "hconcat": [
    {
      "facet": {
        "column": {
          "field": "date",
          "type": "ordinal"
        }
      },
      "spec": {
        "encoding": {
          "x": {
            "field": "x",
            "type": "quantitative"
          },
          "y": {
            "field": "a",
            "type": "quantitative"
          }
        },
        "mark": "bar"
      }
    },
    {
      "facet": {
        "column": {
          "field": "date",
          "type": "ordinal"
        }
      },
      "spec": {
        "encoding": {
          "x": {
            "field": "x",
            "type": "quantitative"
          },
          "y": {
            "field": "b",
            "type": "quantitative"
          }
        },
        "mark": "bar"
      }
    }
  ]
}

Isue 5475 example

{
  "$schema": "https://vega.github.io/schema/vega-lite/v4.json",
  "data": {"values": [{"a": "A", "b": 28}, {"a": "B", "b": 55}]},
  "vconcat": [
    {
      "title": "Title for first row",
      "facet": {"field": "a", "type": "ordinal"},
      "spec": {
        "mark": "bar",
        "encoding": {"y": {"field": "b", "type": "quantitative"}}
      }
    },
    {
      "title": "Title for second row",
      "facet": {"field": "a", "type": "ordinal"},
      "spec": {
        "mark": "bar",
        "encoding": {"y": {"field": "b", "type": "quantitative"}}
      }
    }
  ]
}

Isue 5261 example

{
  "$schema": "https://vega.github.io/schema/vega-lite/v3.json",
  "data": {"url": "data/cars.json"},
  "vconcat": [
    {
      "facet": {"field": "Origin", "type": "nominal"},
      "spec": {
        "mark": "point",
        "encoding": {
          "x": {"field": "Horsepower", "type": "quantitative"},
          "y": {"field": "Miles_per_Gallon", "type": "quantitative"}
        }
      }
    },
    {
      "facet": {"field": "Origin", "type": "nominal"},
      "spec": {
        "mark": "tick",
        "encoding": {
          "x": {"field": "Horsepower", "type": "quantitative"}
        }
      }
    }
  ]
}

[domoritz]

One comment. Other than that, looks great.

Thanks for your patience with my review.

[RobinL]

Thanks. Happy with your suggestion. That was just my lack of understanding of the internals, your version is clearer.

I haven't clicked 'commit suggestion' in the GitHub UI just because I don't want to mess up the house style for commit messages. Happy for you to commit them, or I can batch them into a commit. Would a commit message of fix: preserve node order in swapwithparents (same as my original commit) be suitable?

[domoritz]

We will squash the commits anyway so the individual commit messages are not as critical.