Skip to content

Commit c4f064d

Browse files
fix(drop): drop an auto-populated table's jobs table with it
drop_quick() dropped only the table itself, so drop() left an auto-populated table's jobs table, ~~<table_name>, in place with its records and its primary key. A table later declared under the same name picked it up through its jobs property. When the new primary key was wider, populate(reserve_jobs=True) computed nothing and raised nothing: the old records, keyed on part of the new primary key, covered every new key. drop_quick() now drops the jobs table of a Computed or Imported table after the table itself. drop() reaches each table as a FreeTable, so the tier is read from the table name, as Schema.jobs does. A Manual or Lookup table never drops a jobs table, though a Manual `result` and a Computed `__result` map to the same `~~result`. The jobs table is itself dropped through drop_quick(), so its ~lineage rows and, on PostgreSQL, its enum type go with it. Jobs tables left by earlier releases remain, so the jobs property now checks an existing jobs table's primary key against the one it would be declared with, and raises if they differ, naming the jobs table and the command that removes it. Job computes its foreign-key-derived primary key once, so a table with a primary key attribute of its own still logs the "Ignoring non-FK primary key attribute" warning once per access. The tests run on MySQL and PostgreSQL. In the issue's reproduction, drop() removes ~~result and its lineage rows, and the table declared again with a wider primary key populates all six keys. drop_quick() removes ~~result too, with its enum type on PostgreSQL. Dropping a Manual table leaves the Computed table's ~~result in place. A jobs table left behind by a plain DROP TABLE makes jobs raise; the command in the message removes it, and populate then completes. A table with a primary key attribute of its own passes the check and warns once. Closes #1601.
1 parent a00b89f commit c4f064d

4 files changed

Lines changed: 312 additions & 5 deletions

File tree

‎src/datajoint/autopopulate.py‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -175,16 +175,37 @@ def __get__(self, obj, objtype=None):
175175
-------
176176
Job
177177
Job management object for this table.
178+
179+
Raises
180+
------
181+
DataJointError
182+
If the job table exists with a primary key other than the one
183+
this table's job table is declared with, as when it was left by
184+
an earlier table of the same name.
178185
"""
179186
if obj is None:
180187
# Accessed on class - instantiate first
181188
obj = objtype()
182189
if obj._jobs is None:
183190
from .jobs import Job
184191

185-
obj._jobs = Job(obj)
186-
if not obj._jobs.is_declared:
187-
obj._jobs.declare()
192+
jobs = Job(obj)
193+
if not jobs.is_declared:
194+
jobs.declare()
195+
else:
196+
# A job table keyed on other attributes matches the wrong keys,
197+
# so populate(reserve_jobs=True) would skip work without an error
198+
expected = [name for name, _ in jobs._pk_attrs]
199+
if set(jobs.primary_key) != set(expected):
200+
class_name = type(obj).__name__
201+
raise DataJointError(
202+
f"Jobs table {jobs.full_table_name} has primary key {jobs.primary_key}, "
203+
f"but the jobs table of {obj.full_table_name} needs {expected}: "
204+
f"it was left by an earlier table of the same name. Remove it with "
205+
f"dj.FreeTable({class_name}.connection, {jobs.full_table_name!r}).drop_quick(); "
206+
f"the next access to {class_name}.jobs declares a new one."
207+
)
208+
obj._jobs = jobs
188209
return obj._jobs
189210

190211
jobs: Job = _JobsDescriptor()

‎src/datajoint/jobs.py‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,9 @@ def __init__(self, target_table: Table) -> None:
111111
base_name = target_name.lstrip("_")
112112
self._table_name = f"~~{base_name}"
113113

114-
# Generate definition from target's FK-derived primary key
114+
# The target's FK-derived primary key, computed once: it generates the
115+
# definition and is what an existing jobs table's primary key must match
116+
self._pk_attrs = self._get_fk_derived_pk_attrs()
115117
self._definition = self._generate_definition()
116118

117119
# Initialize heading and support
@@ -147,7 +149,7 @@ def _generate_definition(self) -> str:
147149
str
148150
DataJoint table definition string.
149151
"""
150-
pk_attrs = self._get_fk_derived_pk_attrs()
152+
pk_attrs = self._pk_attrs
151153

152154
if not pk_attrs:
153155
raise DataJointError(

‎src/datajoint/table.py‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1224,6 +1224,8 @@ def delete(
12241224
def drop_quick(self):
12251225
"""
12261226
Drops the table without cascading to dependent tables and without user prompt.
1227+
1228+
An auto-populated table's jobs table (``~~table_name``) is dropped with it.
12271229
"""
12281230
if self.is_declared:
12291231
# Clean up lineage entries for this table
@@ -1256,6 +1258,18 @@ def drop_quick(self):
12561258
logger.debug("Dropped enum type %s" % enum_type)
12571259
except Exception:
12581260
pass # Ignore errors - type may be used by other tables
1261+
1262+
# Drop the jobs table of an auto-populated table, else a table later
1263+
# declared under the same name inherits it. drop() reaches each table
1264+
# as a FreeTable, so the tier is read from the name, as Schema.jobs does;
1265+
# a Manual `result` and a Computed `__result` map to the same `~~result`.
1266+
from .user_tables import Computed, Imported, _get_tier
1267+
1268+
if _get_tier(self.full_table_name) in (Computed, Imported):
1269+
jobs_table_name = "~~" + self.table_name.lstrip("_")
1270+
jobs = FreeTable(self.connection, self.adapter.make_full_table_name(self.database, jobs_table_name))
1271+
if jobs.is_declared:
1272+
jobs.drop_quick()
12591273
else:
12601274
logger.info("Nothing to drop: table %s is not declared" % self.full_table_name)
12611275

Lines changed: 270 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,270 @@
1+
"""
2+
Tests that dropping an auto-populated table drops its jobs table, and that
3+
``jobs`` rejects a jobs table left by an earlier table of the same name (#1601).
4+
"""
5+
6+
import logging
7+
import re
8+
import time
9+
10+
import pytest
11+
12+
import datajoint as dj
13+
from datajoint.errors import DataJointError
14+
from datajoint.lineage import get_table_lineages
15+
16+
17+
@pytest.fixture(scope="function")
18+
def schema_by_backend(connection_by_backend, db_creds_by_backend):
19+
"""Create a fresh schema per test, parameterized across backends."""
20+
backend = db_creds_by_backend["backend"]
21+
test_id = str(int(time.time() * 1000))[-8:]
22+
schema = dj.Schema(f"djtest_dropjobs_{backend}_{test_id}"[:64], connection=connection_by_backend)
23+
yield schema
24+
schema.drop(prompt=False)
25+
26+
27+
def _table_names(schema):
28+
"""Every table in the schema, hidden ones included."""
29+
sql = schema.connection.adapter.list_tables_sql(schema.database)
30+
return {row[0] for row in schema.connection.query(sql).fetchall()}
31+
32+
33+
def _enum_types(schema, table_name):
34+
"""PostgreSQL enum types used by a table's columns (none on MySQL)."""
35+
adapter = schema.connection.adapter
36+
if not hasattr(adapter, "get_table_enum_types_sql"):
37+
return set()
38+
sql = adapter.get_table_enum_types_sql(schema.database, table_name)
39+
return {row[0] for row in schema.connection.query(sql).fetchall()}
40+
41+
42+
def _existing_enum_types(schema):
43+
"""Enum types defined in the schema's namespace (PostgreSQL only)."""
44+
sql = (
45+
"SELECT n.nspname || '.' || t.typname FROM pg_catalog.pg_type t "
46+
"JOIN pg_catalog.pg_namespace n ON n.oid = t.typnamespace "
47+
"WHERE t.typtype = 'e' AND n.nspname = %s"
48+
)
49+
return {row[0] for row in schema.connection.query(sql, args=(schema.database,)).fetchall()}
50+
51+
52+
def test_drop_removes_jobs_table(schema_by_backend):
53+
"""drop() removes the jobs table, so a table declared again under the same name populates in full."""
54+
schema = schema_by_backend
55+
56+
@schema
57+
class Item(dj.Manual):
58+
definition = """
59+
item : int32
60+
"""
61+
62+
@schema
63+
class Param(dj.Lookup):
64+
definition = """
65+
param : int32
66+
"""
67+
contents = [(1,), (2,)]
68+
69+
@schema
70+
class Result(dj.Computed):
71+
definition = """
72+
-> Item
73+
---
74+
value : int32
75+
"""
76+
77+
def make(self, key):
78+
raise RuntimeError("not ready")
79+
80+
Item.insert({"item": i} for i in range(3))
81+
Result.populate(reserve_jobs=True, suppress_errors=True)
82+
assert len(Result.jobs.errors) == 3
83+
assert get_table_lineages(schema.connection, schema.database, "~~result")
84+
85+
Result.drop(prompt=False)
86+
assert "~~result" not in _table_names(schema)
87+
assert not get_table_lineages(schema.connection, schema.database, "~~result")
88+
89+
@schema
90+
class Result(dj.Computed): # same name, primary key now includes Param
91+
definition = """
92+
-> Item
93+
-> Param
94+
---
95+
value : int32
96+
"""
97+
98+
def make(self, key):
99+
self.insert1(dict(key, value=1))
100+
101+
assert Result.jobs.primary_key == ["item", "param"]
102+
assert len(Result.jobs) == 0
103+
Result.populate(reserve_jobs=True)
104+
assert len(Result()) == len(Result.key_source) == 6
105+
106+
107+
def test_drop_quick_removes_jobs_table(schema_by_backend):
108+
"""drop_quick() removes the jobs table with its ~lineage rows and, on PostgreSQL, its enum type."""
109+
schema = schema_by_backend
110+
111+
@schema
112+
class Item(dj.Manual):
113+
definition = """
114+
item : int32
115+
"""
116+
117+
@schema
118+
class Result(dj.Computed):
119+
definition = """
120+
-> Item
121+
---
122+
value : int32
123+
"""
124+
125+
def make(self, key):
126+
self.insert1(dict(key, value=key["item"]))
127+
128+
Item.insert({"item": i} for i in range(3))
129+
Result.jobs.refresh()
130+
assert "~~result" in _table_names(schema)
131+
enum_types = _enum_types(schema, "~~result")
132+
assert bool(enum_types) == (schema.connection.adapter.backend == "postgresql")
133+
134+
Result.drop_quick()
135+
tables = _table_names(schema)
136+
assert "__result" not in tables
137+
assert "~~result" not in tables
138+
assert not get_table_lineages(schema.connection, schema.database, "~~result")
139+
if enum_types:
140+
assert not enum_types & _existing_enum_types(schema)
141+
142+
143+
def test_drop_manual_table_keeps_jobs_table(schema_by_backend):
144+
"""Dropping a Manual table leaves the jobs table its name maps to, which belongs to a Computed table."""
145+
schema = schema_by_backend
146+
147+
@schema
148+
class Item(dj.Manual):
149+
definition = """
150+
item : int32
151+
"""
152+
153+
@schema
154+
class Result(dj.Computed):
155+
definition = """
156+
-> Item
157+
---
158+
value : int32
159+
"""
160+
161+
def make(self, key):
162+
self.insert1(dict(key, value=key["item"]))
163+
164+
computed = Result
165+
Item.insert({"item": i} for i in range(3))
166+
computed.jobs.refresh()
167+
assert len(computed.jobs) == 3
168+
169+
@schema
170+
class Result(dj.Manual): # table `result`; `~~result` belongs to `__result`
171+
definition = """
172+
result_id : int32
173+
"""
174+
175+
Result.drop(prompt=False)
176+
tables = _table_names(schema)
177+
assert "result" not in tables
178+
assert {"__result", "~~result"} <= tables
179+
assert len(computed.jobs) == 3
180+
181+
182+
def test_jobs_rejects_stale_jobs_table(schema_by_backend):
183+
"""jobs raises on a jobs table keyed differently from the table's, and its hint removes it."""
184+
schema = schema_by_backend
185+
186+
@schema
187+
class Item(dj.Manual):
188+
definition = """
189+
item : int32
190+
"""
191+
192+
@schema
193+
class Param(dj.Lookup):
194+
definition = """
195+
param : int32
196+
"""
197+
contents = [(1,), (2,)]
198+
199+
@schema
200+
class Result(dj.Computed):
201+
definition = """
202+
-> Item
203+
---
204+
value : int32
205+
"""
206+
207+
def make(self, key):
208+
raise RuntimeError("not ready")
209+
210+
Item.insert({"item": i} for i in range(3))
211+
Result.populate(reserve_jobs=True, suppress_errors=True)
212+
jobs_table = Result.jobs.full_table_name
213+
# Earlier releases dropped the table and left its jobs table; drop it the same way
214+
schema.connection.query(f"DROP TABLE {Result.full_table_name}")
215+
216+
@schema
217+
class Result(dj.Computed): # same name, primary key now includes Param
218+
definition = """
219+
-> Item
220+
-> Param
221+
---
222+
value : int32
223+
"""
224+
225+
def make(self, key):
226+
self.insert1(dict(key, value=1))
227+
228+
result = Result()
229+
with pytest.raises(DataJointError, match="left by an earlier table of the same name") as excinfo:
230+
result.jobs
231+
message = str(excinfo.value)
232+
assert jobs_table in message
233+
assert "['item']" in message and "['item', 'param']" in message
234+
assert result._jobs is None # the mismatched jobs table is not kept
235+
236+
# The hint runs as written
237+
hint = re.search(r"dj\.FreeTable\(.*\)\.drop_quick\(\)", message).group(0)
238+
eval(hint, {"dj": dj, "Result": Result})
239+
assert result.jobs.primary_key == ["item", "param"]
240+
Result.populate(reserve_jobs=True)
241+
assert len(Result()) == len(Result.key_source) == 6
242+
243+
244+
def test_jobs_accepts_table_with_native_key_attribute(schema_by_backend, caplog):
245+
"""A jobs table keyed on the FK-derived part of the primary key passes the check, warning once."""
246+
schema = schema_by_backend
247+
248+
@schema
249+
class Item(dj.Manual):
250+
definition = """
251+
item : int32
252+
"""
253+
254+
with dj.config.override(jobs__allow_new_pk_fields_in_computed_tables=True):
255+
256+
@schema
257+
class Result(dj.Computed):
258+
definition = """
259+
-> Item
260+
index : int32
261+
---
262+
value : int32
263+
"""
264+
265+
Result.jobs.refresh() # declares the jobs table, keyed on item alone
266+
caplog.clear()
267+
with caplog.at_level(logging.WARNING, logger="datajoint"):
268+
assert Result.jobs.primary_key == ["item"]
269+
warnings = [r for r in caplog.records if "Ignoring non-FK primary key attribute 'index'" in r.getMessage()]
270+
assert len(warnings) == 1

0 commit comments

Comments
 (0)