Skip to content

Commit 1dee94d

Browse files
fix(declare): group only owned parts with their masters
A part is owned by its master when a foreign key in its primary key references the master, or another owned part of the same master; the reference may rename the attributes. Grouping parts by table name alone made topo_sort fail on schemas with no cycle (a part that does not reference its master but references a table downstream of it) and accept a cycle through a part that references its master only through a nullable or secondary foreign key. - owned_parts() reads ownership from the dependency graph through each edge's primary flag; is_owned() decides it for a part being declared. - topo_sort keeps only owned parts with their masters; other parts are sorted as separate tables. group_graph, group_ancestors and group_cycle use owned parts, so a reference to a part the master does not own counts as a reference outside the group. - Part._declare_check checks only owned parts for cycles and warns on a part that is not owned. The foreign-key records drop nullability. - The cascade's sort scope keeps following the name-based part-to-master lift of the current part_integrity engine.
1 parent ba5d72d commit 1dee94d

8 files changed

Lines changed: 569 additions & 163 deletions

File tree

‎src/datajoint/autopopulate.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ def _declare_check(
193193
self,
194194
primary_key: list[str],
195195
fk_attribute_map: dict[str, tuple[str, str]],
196-
foreign_keys: list[tuple[str, dict[str, str], bool]],
196+
foreign_keys: list[tuple[str, dict[str, str]]],
197197
) -> None:
198198
"""
199199
Validate FK-only primary key constraint for auto-populated tables.
@@ -209,7 +209,7 @@ def _declare_check(
209209
fk_attribute_map : dict
210210
Mapping of child_attr -> (parent_table, parent_attr).
211211
foreign_keys : list
212-
One ``(parent_table, attr_map, nullable)`` entry per foreign key.
212+
One ``(parent_table, attr_map)`` entry per foreign key.
213213
Not used here.
214214
215215
Raises

‎src/datajoint/declare.py‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ def compile_foreign_key(
204204
fk_attribute_map: dict[str, tuple[str, str]] | None = None,
205205
fk_index_candidates: list[list[str]] | None = None,
206206
column_comments: dict[str, str] | None = None,
207-
foreign_keys: list[tuple[str, dict[str, str], bool]] | None = None,
207+
foreign_keys: list[tuple[str, dict[str, str]]] | None = None,
208208
) -> None:
209209
"""
210210
Parse a foreign key line and update declaration components.
@@ -241,7 +241,7 @@ def compile_foreign_key(
241241
comment, with its core-type marker (e.g. ``:int32:``), is added so the
242242
child keeps the parent's declared type. Updated in place.
243243
foreign_keys : list, optional
244-
One ``(parent_table, attr_map, nullable)`` entry per foreign key, where
244+
One ``(parent_table, attr_map)`` entry per foreign key, where
245245
``attr_map`` maps each child attribute to the parent attribute it
246246
references. Unlike ``fk_attribute_map``, it keeps every reference when
247247
several share an attribute. Updated in place.
@@ -335,7 +335,7 @@ def compile_foreign_key(
335335
)
336336
if foreign_keys is not None:
337337
attr_map = {attr: ref.heading[attr].original_name for attr in ref.primary_key}
338-
foreign_keys.append((ref_table_name, attr_map, is_nullable))
338+
foreign_keys.append((ref_table_name, attr_map))
339339

340340
# Supporting index on the foreign-key columns.
341341
#
@@ -397,7 +397,7 @@ def prepare_declare(
397397
*,
398398
table_name: str | None = None,
399399
config=None,
400-
foreign_keys: list[tuple[str, dict[str, str], bool]] | None = None,
400+
foreign_keys: list[tuple[str, dict[str, str]]] | None = None,
401401
) -> tuple[str, list[str], list[str], list[str], list[str], list[str], dict[str, tuple[str, str]], dict[str, str]]:
402402
"""
403403
Parse a table definition into its components.
@@ -423,7 +423,7 @@ def prepare_declare(
423423
config : Config, optional
424424
Read for ``jobs.add_job_metadata`` and ``provenance.capture``.
425425
foreign_keys : list, optional
426-
Receives one ``(parent_table, attr_map, nullable)`` entry per foreign
426+
Receives one ``(parent_table, attr_map)`` entry per foreign
427427
key; see :func:`compile_foreign_key`. Updated in place.
428428
429429
Returns
@@ -559,7 +559,7 @@ def declare(
559559
adapter,
560560
*,
561561
config=None,
562-
foreign_keys: list[tuple[str, dict[str, str], bool]] | None = None,
562+
foreign_keys: list[tuple[str, dict[str, str]]] | None = None,
563563
) -> tuple[str, list[str], list[str], dict[str, tuple[str, str]], list[str], list[str]]:
564564
r"""
565565
Parse a definition and generate SQL CREATE TABLE statement.
@@ -577,7 +577,7 @@ def declare(
577577
config : Config, optional
578578
Configuration object. If None, falls back to global config.
579579
foreign_keys : list, optional
580-
Receives one ``(parent_table, attr_map, nullable)`` entry per foreign
580+
Receives one ``(parent_table, attr_map)`` entry per foreign
581581
key; see :func:`compile_foreign_key`. Updated in place.
582582
583583
Returns

‎src/datajoint/dependencies.py‎

Lines changed: 120 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -44,44 +44,118 @@ def extract_master(part_table: str) -> str | None:
4444
CHAIN_LEGEND = "each table references the one before it or is the master of the part before it"
4545

4646

47-
def group_graph(graph: nx.MultiDiGraph) -> nx.MultiDiGraph:
47+
def owned_parts(graph: nx.MultiDiGraph) -> set[str]:
4848
"""
49-
Copy a dependency graph and add an edge from each part table to its master.
49+
Return the part tables that are owned by their masters.
5050
51-
A master's rows are inserted and deleted together with its parts' rows, so a
52-
master depends on everything its parts depend on. With the added edges,
53-
``nx.ancestors`` gives a table's ancestors where reaching a master also
54-
reaches all of its parts, and ``nx.descendants`` gives the converse, where
55-
reaching a part also reaches its master. These are the same dependencies that
56-
:func:`topo_sort` orders.
51+
A part is owned by its master when a foreign key in the part's primary key
52+
references the master, or references another owned part of the same master.
53+
The reference may rename the attributes. Only owned parts form a group with
54+
their master; every other part (one with no reference to its master, or only a
55+
secondary or nullable one, or only one through a part that is not owned) is
56+
treated as a separate table.
5757
58-
The added edges form a cycle with each part that references its master, so
59-
the result is for reachability only, not for sorting.
58+
Each foreign key is its own edge, renamed ones included, and carries a
59+
``primary`` flag that is True when all its attributes are in the referencing
60+
table's primary key. An edge without the flag, as in a graph built by hand,
61+
counts as primary.
6062
6163
Parameters
6264
----------
6365
graph : nx.MultiDiGraph
6466
Dependency graph.
6567
68+
Returns
69+
-------
70+
set[str]
71+
Full names of the owned parts.
72+
"""
73+
masters = {extract_master(node) for node in graph.nodes} & set(graph.nodes)
74+
owned = set()
75+
for master in masters:
76+
frontier = [master]
77+
while frontier:
78+
for _, child, props in graph.out_edges(frontier.pop(), data=True):
79+
if child not in owned and extract_master(child) == master and props.get("primary", True):
80+
owned.add(child)
81+
frontier.append(child)
82+
return owned
83+
84+
85+
def _in_group(table: str, master: str, owned: set[str]) -> bool:
86+
"""Whether ``table`` is ``master`` or one of its owned parts."""
87+
return table == master or (table in owned and extract_master(table) == master)
88+
89+
90+
def is_owned(graph: nx.MultiDiGraph, part: str, references: list[tuple[str, bool]]) -> bool:
91+
"""
92+
Tell whether a new part table will be owned by its master (see :func:`owned_parts`).
93+
94+
Parameters
95+
----------
96+
graph : nx.MultiDiGraph
97+
Dependency graph of the declared tables, which does not include ``part``.
98+
part : str
99+
Full name of the part table being declared.
100+
references : list[tuple[str, bool]]
101+
One ``(parent_table, primary)`` pair per foreign key of the part, where
102+
``primary`` is True when all the foreign key's attributes are in the
103+
part's primary key.
104+
105+
Returns
106+
-------
107+
bool
108+
True if a primary-key reference goes to the master or to an owned part of it.
109+
"""
110+
master = extract_master(part)
111+
if master not in graph:
112+
return False
113+
owned = owned_parts(graph)
114+
return any(primary and _in_group(parent, master, owned) for parent, primary in references)
115+
116+
117+
def group_graph(graph: nx.MultiDiGraph, owned: set[str] | None = None) -> nx.MultiDiGraph:
118+
"""
119+
Copy a dependency graph and add an edge from each owned part to its master.
120+
121+
A master's rows are inserted and deleted together with its owned parts' rows,
122+
so a master depends on everything its owned parts depend on. With the added
123+
edges, ``nx.ancestors`` gives a table's ancestors where reaching a master also
124+
reaches all of its owned parts, and ``nx.descendants`` gives the converse,
125+
where reaching an owned part also reaches its master. These are the
126+
dependencies that :func:`topo_sort` orders. Parts that are not owned are
127+
treated as separate tables.
128+
129+
The added edges form a cycle with each owned part, so the result is for
130+
reachability only, not for sorting.
131+
132+
Parameters
133+
----------
134+
graph : nx.MultiDiGraph
135+
Dependency graph.
136+
owned : set[str], optional
137+
The owned parts, if already computed with :func:`owned_parts`.
138+
66139
Returns
67140
-------
68141
nx.MultiDiGraph
69-
A copy of ``graph`` with an edge from each part to its master.
142+
A copy of ``graph`` with an edge from each owned part to its master.
70143
"""
144+
owned = owned_parts(graph) if owned is None else owned
71145
graph = nx.MultiDiGraph(graph)
72146
for part in list(graph.nodes):
73-
if (master := extract_master(part)) in graph:
74-
graph.add_edge(part, master, key="master")
147+
if part in owned:
148+
graph.add_edge(part, extract_master(part), key="master")
75149
return graph
76150

77151

78152
def group_ancestors(graph: nx.MultiDiGraph, table: str) -> set[str]:
79153
"""
80-
Return a table and its ancestors, where reaching a master reaches all its parts.
154+
Return a table and its ancestors, where reaching a master reaches all its owned parts.
81155
82156
The closure starts from ``table`` and adds the parents of every table in it,
83-
and all the parts of every master in it, until nothing new is added. A part
84-
reaches its own master through its reference to the master (``-> master``).
157+
and all the owned parts of every master in it, until nothing new is added. An
158+
owned part reaches its master through its own references.
85159
86160
Parameters
87161
----------
@@ -100,24 +174,26 @@ def group_ancestors(graph: nx.MultiDiGraph, table: str) -> set[str]:
100174

101175
def group_cycle(graph: nx.MultiDiGraph, part: str, parents: list[str]) -> list[str] | None:
102176
"""
103-
Find the cycle that a new part table's references would close.
177+
Find the cycle that the references of a new owned part would close.
104178
105-
A part may reference its master, another part of the same master, or a table
106-
outside its master-part group. A reference to a table outside the group closes
107-
a cycle when that table already depends on the part's master (see
108-
:func:`group_ancestors`): the group would then depend on itself, and
109-
:func:`topo_sort` could no longer order its tables.
179+
An owned part may reference its master, an owned part of the same master, or
180+
a table outside its master-part group, which includes the master's parts that
181+
are not owned. A reference to a table outside the group closes a cycle when
182+
that table already depends on the part's master (see :func:`group_ancestors`):
183+
the group would then depend on itself, and :func:`topo_sort` could no longer
184+
order its tables.
110185
111-
A new table has no children yet, so only a new part's references to tables
112-
outside its group can close such a cycle, and checking them when the part is
113-
declared keeps every cycle out.
186+
A new table has no children yet, so only the references of a new owned part
187+
to tables outside its group can close such a cycle. A part that is not owned
188+
is a separate table and closes none (see :func:`is_owned`). Checking owned
189+
parts when they are declared therefore keeps every cycle out.
114190
115191
Parameters
116192
----------
117193
graph : nx.MultiDiGraph
118194
Dependency graph of the declared tables, which does not include ``part``.
119195
part : str
120-
Full name of the part table being declared.
196+
Full name of the owned part table being declared.
121197
parents : list[str]
122198
Full names of the tables the part references.
123199
@@ -130,10 +206,11 @@ def group_cycle(graph: nx.MultiDiGraph, part: str, parents: list[str]) -> list[s
130206
master = extract_master(part)
131207
if master not in graph:
132208
return None
133-
outside = [p for p in parents if p in graph and master not in (p, extract_master(p))]
209+
owned = owned_parts(graph)
210+
outside = [p for p in parents if p in graph and not _in_group(p, master, owned)]
134211
if not outside:
135212
return None
136-
extended = group_graph(graph)
213+
extended = group_graph(graph, owned)
137214
for parent in outside:
138215
try:
139216
path = nx.shortest_path(extended, master, parent)
@@ -190,7 +267,11 @@ def _cycle_error(graph: nx.MultiDiGraph) -> DataJointError:
190267

191268
def topo_sort(graph: nx.MultiDiGraph, nodes=None) -> list[str]:
192269
"""
193-
Topological sort keeping part tables with their masters.
270+
Topological sort keeping owned part tables with their masters.
271+
272+
Each owned part (see :func:`owned_parts`) is placed right after its master, so
273+
the master is ordered after every table its owned parts reference. Parts that
274+
are not owned are sorted as separate tables.
194275
195276
Parameters
196277
----------
@@ -205,14 +286,15 @@ def topo_sort(graph: nx.MultiDiGraph, nodes=None) -> list[str]:
205286
Returns
206287
-------
207288
list[str]
208-
Table names in topological order with parts following masters.
289+
Table names in topological order with owned parts following their masters.
209290
210291
Raises
211292
------
212293
DataJointError
213294
If the dependencies of the tables to sort form a cycle, for example one
214-
that runs through a master and its parts.
295+
that runs through a master and its owned parts.
215296
"""
297+
owned = owned_parts(graph)
216298
if nodes is None:
217299
graph = nx.MultiDiGraph(graph) # make a mutable copy
218300
else:
@@ -225,15 +307,14 @@ def topo_sort(graph: nx.MultiDiGraph, nodes=None) -> list[str]:
225307
subgraph.add_edges_from((u, v) for u, v in graph.edges(keep) if v in keep)
226308
graph = subgraph
227309

228-
# Add parts' dependencies to their masters' dependencies
310+
# Add owned parts' dependencies to their masters' dependencies
229311
# to ensure correct topological ordering of the masters.
312+
# Parts that are not owned are sorted as separate tables.
230313
for part in graph:
231-
# find the part's master
232-
if (master := extract_master(part)) in graph:
233-
for edge in graph.in_edges(part):
234-
parent = edge[0]
235-
if master not in (parent, extract_master(parent)):
236-
# if parent is neither master nor part of master
314+
if part in owned and (master := extract_master(part)) in graph:
315+
for parent, _ in graph.in_edges(part):
316+
if not _in_group(parent, master, owned):
317+
# if parent is neither master nor owned part of master
237318
graph.add_edge(parent, master, part=part)
238319
try:
239320
sorted_nodes = list(nx.topological_sort(graph))
@@ -245,7 +326,7 @@ def topo_sort(graph: nx.MultiDiGraph, nodes=None) -> list[str]:
245326
placed = set()
246327
while pos > 1:
247328
part = sorted_nodes[pos]
248-
if (master := extract_master(part)) not in graph or part in placed:
329+
if part not in owned or (master := extract_master(part)) not in graph or part in placed:
249330
pos -= 1
250331
else:
251332
placed.add(part)

‎src/datajoint/diagram.py‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
import networkx as nx
2525

2626
from .condition import AndList
27-
from .dependencies import extract_master, group_graph, topo_sort
27+
from .dependencies import extract_master, topo_sort
2828
from .errors import DataJointError
2929
from .table import Table, lookup_class_name
3030
from .user_tables import Computed, Imported, Lookup, Manual, Part, _get_tier
@@ -781,12 +781,18 @@ def _propagate_restrictions(self, start_node, mode, part_integrity="enforce"):
781781

782782
# Only propagate through descendants of start_node
783783
allowed_nodes = {start_node} | set(nx.descendants(self, start_node))
784-
# Sort only the tables propagation can reach: part_integrity="cascade" also
785-
# reaches the master of every part it reaches, and the master's descendants.
784+
# Sort only the tables propagation can reach. With part_integrity="cascade",
785+
# propagation also reaches the master of every part it reaches (matched by
786+
# name, as in _propagate_part_to_master), and the master's descendants.
787+
reachable = set(allowed_nodes)
786788
if part_integrity == "cascade" and mode == "cascade":
787-
reachable = {start_node} | nx.descendants(group_graph(self), start_node)
788-
else:
789-
reachable = allowed_nodes
789+
pending = list(reachable)
790+
while pending:
791+
master = extract_master(pending.pop())
792+
if master in self.nodes and master not in reachable:
793+
added = {master} | nx.descendants(self, master)
794+
pending.extend(added - reachable)
795+
reachable |= added
790796
sorted_nodes = topo_sort(self, reachable)
791797
propagated_edges = set()
792798
# Dedup upward part→master walks by (part, master) PAIR, not by master

‎src/datajoint/table.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ def _declare_check(self, primary_key, fk_attribute_map, foreign_keys):
219219
fk_attribute_map : dict
220220
Dict mapping child_attr -> (parent_table, parent_attr).
221221
foreign_keys : list
222-
One ``(parent_table, attr_map, nullable)`` entry per foreign key,
222+
One ``(parent_table, attr_map)`` entry per foreign key,
223223
where ``attr_map`` maps child_attr -> parent_attr.
224224
"""
225225
pass # Default: no validation

0 commit comments

Comments
 (0)