Propagate upstream changes into downstream cubes - #2424
Open
shangyian wants to merge 9 commits into
Open
Conversation
A cube was filtered out of downstream propagation, and even reached it would only have had its status revalidated -- nothing computed it a version. So an upstream edit left the cube pointing at a materialized table built against a definition that no longer existed, and the only way to recompile it was a no-op edit to the cube itself. Propagation now includes cubes and gives each one a new revision resolved from its own current metrics and dimensions, so its columns, elements and parents recompile against what the upstream became. The revision is built by the same code the cube update endpoint uses, extracted as `save_new_cube_revision`, so the two writers cannot produce differently-shaped revisions, and it swaps materializations the same way so the superseded revision's workflows are stopped rather than left pointing at a dead revision. The cube inherits the upstream's change tier rather than being classified on its own shape. Widening a transform's WHERE clause changes no metric, no dimension and no metric component identity -- `is_non_trivial_cube_change` returns False for it -- yet every row in the cube's table was computed under the old filter. Over-rebuilding costs compute; under-rebuilding serves wrong numbers. A cube that cannot be rebuilt is logged and skipped rather than costing the remaining downstreams their propagation.
`revalidate_node` bumped a node's major version for any difference between its stored columns and what the validator recomputed. A column that only just appeared cannot be referenced by anything written before it existed, so an additive change breaks nobody, and now that downstream cubes inherit their upstream's tier a major bump there rebuilds every cube's materialized table for it. A column whose type moved stays major: everything reading it is reading a different type than it was written against. `order_fixed` -- DJ backfilling a missing `order` onto stored columns to match the query's projection -- is filed with the additive cases. It changes no name, no type and no value, so it arguably deserves no revision at all; minor is the conservative reading, since a revision is what the existing audit-trail test expects it to produce. The decision is a `ChangeTier` handed to `bump_version` rather than a direct call to `next_major_version`, so it is a value propagation can pass on to downstream cubes.
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
`order_fixed` fires when a stored column has no `order` and DJ fills in the projection index -- its own bookkeeping on a row written before the field existed. No query changed and no name, type or value moved, so it earned a version bump that described nothing, and since downstream cubes inherit their upstream's tier, it would now rebuild every cube's materialized table for a metadata fix. It is `ChangeTier.NONE`, and the fix is applied to the current revision in place. Leaving it unset is not free: readers sort columns by `order` with unset last, so an unordered column drifts to the end of the projection and every `to_spec` logs the node as unordered. Writing the projection index onto the stored row misrepresents nothing, since the rows were inserted in that order to begin with -- and where a revision is only partly ordered, it puts a column back where the query always projected it. The audit trail survives the loss of the bump. A backfill still writes a history event naming the columns it filled and the version the node kept, because nothing else would record that DJ rewrote the row. Three existing tests used a cleared column order as a cheap way to force revalidation to fork a revision, which it no longer does; they now disagree with a stored column type instead, so they keep testing the fork path rather than passing vacuously. Also documents, next to the cube rebuild check, why the churn there is deliberate: any query edit is a major bump, so every upstream query edit rebuilds every cube below it. Narrowing that by diffing resolved output columns was considered and rejected -- a query edit can move a filter, a join or a CASE threshold while leaving every column and type identical, and each changes every row served.
The merge loop walked the validator's columns and looked each up in the stored ones, so it could only ever find what the validator produced. A column the revision still stores but the query no longer selects was never visited: no bump, and it rode onto the next revision advertising a value the node cannot supply. describe_column_changes already compares both directions, so use it. A removal is major -- anything referencing the column is broken. That occasionally rebuilds a downstream cube that never used it, which is affordable because removals are rare and additions are not.
An order-only change no longer earns a revision, so the in-loop backfill and the history detail that reports it are reachable only when a revision is created for another reason and a legacy column also has no order. The suite exercised those separately and never together.
Dropping a column a metric aggregates leaves the transform valid -- its own query is fine -- while the metric can no longer infer a type and the cube built on it follows. Each bumps, so nothing is left serving a definition that no longer resolves. Worth pinning because the failure is only visible downstream: the edit that causes it looks entirely successful at the node being edited.
previous_table_usable was derived from is_non_trivial_cube_change on every path, including propagation -- so a filter change upstream, the case that motivated not using that predicate to decide rebuilds, recorded the old table as reusable exactly when its data is what went stale. Propagation now passes False, since identical shapes say nothing about the rows. Also: correct the claim that both cube-revision writers route through here (deploy builds and swaps on its own path), say why node survives the recovery rollback, and cover a cube failing after one already succeeded.
The sibling of test_patch_and_deployment_agree_on_version, which only covers edits to the cube itself. Here the cube's spec is byte-identical across two deploys and only the metric under it changes, which filter_nodes_to_deploy cannot see -- an unchanged spec is skipped, so the cube keeps a revision compiled against the old definition while the PATCH path propagates into it. Fails on the cube assertion (v2.0 on PATCH, v1.0 on deploy); the metric assertion above it passes, so the divergence is the cube alone. Red until the deploy path shares the propagation bump.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changing a node in a material sense doesn't bump the version of the cubes downstream from it. The cube keeps serving a materialized table built against the old upstream, and the only way to fix it is pushing a no-op PR against the cube to force a recompile.
This is because
_propagate_update_downstreamcallsget_downstream_nodes(..., include_cubes=False), so cubes are filtered out of the downstream list before the loop runs. And even if they weren't, the loop only callsrevalidate_node, whose cube branch sets status and returns, with no bump logic like that inupdate_cube_node.Changes:
update_cube_nodeis extracted intosave_new_cube_revision, so the update path and propagation build a cube revision the same way rather than drifting.Test Plan
Deployment Plan
An upstream edit can now trigger rematerialization of downstream cubes, which didn't happen before. Cube bumps also stop the superseded revision's workflows via the existing swap.