direct: collapse block+leaf overlap to one level in the plan - #6624
Open
denik wants to merge 4 commits into
Open
direct: collapse block+leaf overlap to one level in the plan#6624denik wants to merge 4 commits into
denik wants to merge 4 commits into
Conversation
Collaborator
Integration test reportCommit: 84317dc
Top 16 slowest tests (at least 2 minutes):
|
denik
force-pushed
the
denik/plan-one-level
branch
from
September 11, 2026 12:02
8231aa8 to
5e1db42
Compare
denik
changed the title
denik
force-pushed
the
denik/plan-one-level
branch
3 times, most recently
from
September 14, 2026 09:41
3ac9325 to
21b9b66
Compare
denik
marked this pull request as ready for review
September 14, 2026 10:35
denik
requested review from
ilyakuz-db and
janniklasrose
and removed request for
janniklasrose
September 14, 2026 10:35
Contributor
Approval status: pending
|
ilyakuz-db
reviewed
Sep 14, 2026
| @@ -28,3 +28,2 @@ | ||
| clusters: | ||
| - label: maintenance | ||
| - num_workers: 3 |
Contributor
There was a problem hiding this comment.
This one looks like a regression due to less granular updates? Cluster is defined in 2 places. Previously, we were able to detect the change and remove
Not sure though what is desired behavior here
When a whole block is added/removed on one side it is recorded as a single block-level change, and the other diff may also descend to inner fields of the same subtree. prepareChanges now drops any change that has an ancestor change in the map, so the plan carries one level per subtree. config-remote-sync consumes these changes and produces identical config (verified: config_edits/multiple_files write the same YAML, only the redundant child op drops from the op-list; split/positional shifts its documented positional gap). update_mask is intentionally not addressed here (postgres default_endpoint_settings still masks the enclosing message); that is handled by the parallel static-mask work. Co-authored-by: Isaac <no-reply@databricks.com>
Block-wins collapses an edited block to a single block-level plan entry, but the update_mask must still name the changed leaves: masking the whole message makes the API require every field under it in the request body. collectUpdatePathsWithPrefix now re-diffs each block change's Old against New to recover those leaves; a whole add/remove (one side nil) does not descend and masks as the block, matching prior behavior. Co-authored-by: Isaac <no-reply@databricks.com>
The block+leaf overlap collapse dropped a leaf against its block ancestor in prepareChanges, before classification. For a missing-in-remote block (e.g. job_runs' local-only lifecycle fingerprint) the remote diff always emits a whole-block change that the resource then drops (ReasonDrop), while the real recreate rides on the leaf. Collapsing first dropped the leaf, then the ancestor was dropped too, so on_bundle_deploy/on_file_change no longer re-fired the run. Move the collapse to keepOneLevelPerSubtree, run after addPerFieldActions on the classified, drop-pruned map. A dropped ancestor is already gone and can no longer shadow a real leaf; a surviving block ancestor still inherits a descendant's recreate via findMatchingRuleBidirectional, so the genuine overlap cases (config-remote-sync, postgres update_mask) are unchanged. Co-authored-by: Isaac <no-reply@databricks.com>
These goldens track the plan's per-field op list, which the one-level collapse trims: a redundant child op under a block op is dropped (email_notifications.on_failure[0], tasks[...].new_cluster, config.env[0].value), and split/positional shows its documented positional remnant. The written config is otherwise unchanged. Co-authored-by: Isaac <no-reply@databricks.com>
denik
force-pushed
the
denik/plan-one-level
branch
from
September 14, 2026 13:08
21b9b66 to
84317dc
Compare
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
3 participants
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.
Changes
When a whole block is added or removed on one side, structdiff records a single block-level change while the other diff descends to the inner fields, producing an overlap (e.g.
scheduleandschedule.timezone_id).prepareChangesnow drops any change that has an ancestor change in the same resource, so the plan carries one level per subtree (block wins).config-remote-sync produces identical config from the result. Postgres rebuilds its
update_maskfrom the plan, socollectUpdatePathsWithPrefixre-diffs each block change to recover the changed leaves — masking the whole message is rejected by the API.Follows #6589.
Tests
Existing acceptance tests, regenerated.