[dimitri-yatsenko]

Closes #1589, closes #1590, closes #1591.

A cascade delete reached rows outside the cascade in three cases:

• dj.Top seed (delete() ignores a dj.Top restriction and deletes every matching row #1589). Diagram.cascade and Diagram.trace built the seed from the WHERE restriction only, and delete_quick used where_clause() only. A seed restricted by Top is now replaced by the primary keys it selects.
• Copy through a foreign key (Cascade delete removes child rows whose nullable foreign key references nothing #1590). A parent's restriction was copied to the child whenever the restricted attributes were in the child's primary key. Through a nullable foreign key that shares a primary-key column, that also selected child rows whose foreign key is NULL. In a diamond, where the child inherits the attribute through another foreign key, it missed rows. The copy now applies only when the restricted attributes are among the foreign key's attributes and the foreign key lies in the child's primary key. Otherwise the child is restricted by the projected parent.
• Part-to-master lift (part_integrity="cascade" deletes masters that have no matching part rows #1591). With part_integrity="cascade", the lift copied the part's condition to the master, which selected masters with no matching part rows. The master is now restricted by the restricted part, so a master joins the cascade only if one of its part rows did.

Two follow-on changes:

• The part-to-master walk restricts intermediate parts only to carry the restriction up to the master. Those restrictions reference the descendant part, which is deleted first (and on MySQL they raise error 1093). They are now restored once the master is materialized; the master then cascades back to all of its parts.
• The Diagram copy constructor copies each node's restriction list instead of deep-copying it. Projected restrictions are query expressions bound to a live connection and cannot be deep-copied. This affected prune() and the diagram operators after restrict().

Not changed: Diagram.trace keeps its upward copy for now. self.upstream traces from a row that make() has not inserted yet and relies on it. The traversal redesign for 2.4 defines that case explicitly.

Tests: tests/integration/test_cascade_overdelete.py adds four tests on MySQL and PostgreSQL. All fail on v2.3.4 and pass here. The full suite passes locally (1166 passed, 14 skipped).

The same commit is cherry-picked onto maint/2.3 for a v2.3.5 patch release.

[lum-agilyti]

Read through the change against 1589, 1590 and 1591. Each fix seems to close its issue, and the narrower copy rule looks like it leaves the common primary-key chains on the same path as before. A few notes and questions:

• No CI checks appear to have run on this PR or on 1593. Could they be re-triggered before merge?
• Diagram.restrict() (diagram.py:778) still builds its seed from table_expr.restriction, so Diagram(schema).restrict(T & dj.Top(2)) looks like it would keep every row matching the other conditions. It isn't a delete, but it seems to be the same seed issue as 1589. Is it meant to be in scope here, or a follow-up?
• In _propagate_part_to_master, saved keeps self._restriction_attrs.get(node), which is the same set object the walk then extends in place via setdefault(...).update(...). So restoring the attributes seems to be a no-op whenever the intermediate part already had attributes. As far as I can tell the extra attributes only make the copy rules stricter, so the effect looks limited to an extra subquery. The code comment says they are restored, though. Is that intended?
• Two cases don't seem to be covered by the new tests:
  • Diagram.trace(T & dj.Top(n)), although the seed change also applies to trace().
  • A Top that selects no rows. The seed is then [[]] and the delete correctly removes nothing. If the seed were ever empty instead, [restriction] if restriction else [] looks like it would leave it unrestricted.
• Outside this diff, a question about trace(): its upward Rule 1 (diagram.py:983) still copies the child's condition to the parent whenever the restricted attributes are in the parent's primary key. The new copy_allowed docstring notes that copying "would also select parent rows that no child row references", and that seems to apply here too. For example, with a table keyed by subject_id that has a secondary FK to Surgery, self.upstream[Surgery] looks like it would return every surgery of that subject, not just the one the row references. I can see the copy may be deliberate, since inside make() the traced row hasn't been inserted yet. Is this intended?
• 1593 appears to match this PR line for line.

[MilagrosMarin]

Thanks for the thorough write-up @dimitri-yatsenko — the three root causes and the "restrict only what the cascade reaches" framing hold up when I read the diff. I haven't run the suite locally; these notes come from reading the code.

Requested change

✅ Add a regression test for an empty seed. _seed_restriction replaces a Top seed with AndList([table_expr.keys()]). When keys() returns [], this relies on AndList([[]]) being truthy and compiling to FALSE. From condition.py that looks right, but if it were ever treated as "no restriction" the cascade would delete every row, so it's worth pinning down. For example: (Item & dj.Top(2)).delete() on an empty table, with an assertion that the preview counts are 0 and nothing is deleted.

Merge ordering

#1525 (also in milestone v2.3.5) rewrites _propagate_part_to_master, and a trial merge of the two conflicts in diagram.py. Which should land first? Whichever goes second will need the saved/restored intermediate restrictions in this PR re-checked against the all-paths walk.

Questions

• table_expr.keys() pulls every selected key into Python, and the seed becomes a list of dicts that compiles to an OR of conditions. For a large Top (or, after #1611, a large subquery seed), could that exceed max_allowed_packet? Is there a practical bound you'd want to document or test?
• On head 18e5649 I don't see any workflow runs or check-runs through the API. Could you point me to where CI ran?

Everything else reads correct to me: the nullable-FK and diamond rule (parent_attrs <= set(attr_map) <= child_pk), the existential part→master lift, and the copy-constructor change. Let me know if anything needs adjusting!

[dimitri-yatsenko]

Thanks, @lum-agilyti and @MilagrosMarin. Pushed f14d68b here; the same commit is on #1593.

Done in this PR

• Empty Top seed: a new test, test_top_seed_that_selects_no_rows_deletes_nothing. Item & "item_id > 100" & dj.Top(2) previews 0 rows in both tables, and the delete leaves every row in place.
• trace with a Top seed: test_top_seed_in_trace checks that the trace starts from the two rows the Top selects.
• Restoring the walked parts' attributes: you were right that saved held the same set the walk extends in place, so the restore did nothing. It now saves a copy.

Left for 2.4
In 2.4, Diagram.expand replaces the traversal (spec in datajoint/datajoint-docs#249), so this PR stays limited to the three over-delete cases. The points below are covered there:

• Diagram.restrict() with a Top seed: restrict is deprecated in 2.4, and expand materializes Top seeds.
• trace's upward copy: it is deliberate in 2.3. self.upstream traces from the row make() is about to insert, and that row does not exist yet. In 2.4, crossing upward is always a restriction, never a copy, and self.upstream is built from the parents of that row.
• Large key lists and max_allowed_packet: in 2.4, keys are materialized only at delete time and applied in chunks. A large Top here carries the same cost it did before this PR.

Other questions

• CI: the workflow runs from early to mid October were deleted across the repository, and with them the checks of this PR and fix(cascade): restrict only the rows a cascade reaches (2.3) #1593. I reopened both, so the checks are running again.
• fix(diagram): walk all FK paths in the part→master cascade walk #1525 versus this PR: fix(diagram): walk all FK paths in the part→master cascade walk #1525 will be closed unmerged. Its multi-path walk is replaced in 2.4 by a one-step lift, and its test cases move to the shape catalog (test(diagram): reference model and properties for Diagram.expand #1621).