docs(data-ops): insertion discipline, update-data how-to, master-part & make-transaction fixes#226
Conversation
… storage Deleting rows leaves in-store objects (<blob@>/<attach@>/<object@>/<npy@>) in place by design; add a 'Reclaiming Object Storage' note + See-also link to the garbage-collection how-to so readers know deletes don't free external storage.
…ke-transaction fixes - insert-data: add a 'What Not to Insert' section — don't hand-insert into Computed/Imported (rows come from make()/populate()); Lookup rows come from contents; insert parents before children; no transaction inside make(). (#221) - add how-to/update-data.md — updates are rare/surgical; prefer delete+reinsert+ recompute; update1() for single-row corrections. (#223) - master-part spec: state transitive completeness — a dependency on the master is a dependency on all its parts. (#222) - autopopulate spec §5.5: fix contradiction — was showing a transaction opened inside make(); make() is already transactional (nested raises). (#224) - delete-data: cross-link update-data. Closes #221, #222, #223, #224.
MilagrosMarin
left a comment
There was a problem hiding this comment.
Thanks — most of this is solid and verified:
- §5.5 make-transaction fix (#224) is exactly right;
connection.py:523explicitly raises "Nested connections are not supported" ifstart_transaction()fires during an active transaction, and_populate_one(autopopulate.py:664) opens one beforemake(). So the old §5.5 example would raise. insert-data.md"What Not to Insert" claims all accurate (Computed/Imported reject direct inserts via_allow_insert; Lookup rows viacontents; parents-before-children; no-transactions-in-make()).update-data.mdreads cleanly; delete+reinsert vsupdate1()distinction is correct, and the primary-key immutability + no-recompute caveats are right.delete-data.mdGC cross-reference lands accurately.- All CI green; all internal links resolve.
One blocker on the master-part.md §2.1 addition:
The new sentence — "Downstream tables therefore reference the master alone and can rely on the whole composite being present — they do not (and cannot) depend on the parts individually." — the (and cannot) is too strong and contradicts standard DataJoint practice:
- The framework's own tests use downstream FK references to Part tables:
tests/schema_simple.py:126→-> B.Ctests/schema_simple.py:148→-> E.Htests/integration/test_schema.py:251→-> Schema_A.Subjecttests/integration/test_cascade_delete.py:326→-> Master.PartAtests/integration/test_cascade_delete.py:373→-> Master.PartA.proj(...)(renamed-FK case)
- The Elements ecosystem uses the same pattern widely (e.g.,
-> Segmentation.Maskinelement-calcium-imaging).
If the intent is "you don't need to reference the parts individually because a master reference reaches the whole composite," that's a useful design guideline — but "cannot" reads as a hard rule and contradicts a legal, common, and framework-tested pattern. Would you soften along the lines of "typically reference the master alone; a foreign key to a specific Part is legal but often unnecessary when the composite is what matters" or equivalent?
Related, smaller clarification on the same paragraph:
"Because the master and its parts are inserted and deleted as one unit" — deletion is FK-CASCADE enforced, but insertion atomicity is only enforced when make() inserts both (Computed/Imported via populate()'s wrapping transaction). For Manual master-part, users can insert Master.insert1(...) without any Part rows and the RDBMS accepts it; atomic insertion is convention (with dj.conn().transaction: Master.insert1(...); Master.Part.insert(...)), not enforcement. Reads as a mechanical guarantee today — worth a brief clarification either way.
Happy to re-review once §2.1 is adjusted.
…ertion atomicity Per review on #226: a downstream FK to a specific Part is legal and common (framework-tested), so drop the '(and cannot)' framing. Also distinguish make()-enforced insertion atomicity (Computed/Imported) from the convention used for Manual master-part sets.
|
Thanks @MilagrosMarin — addressed both points in `master-part.md` §2.1 (68bf9d9):
Ready for re-review. |
MilagrosMarin
left a comment
There was a problem hiding this comment.
Thanks — both changes read exactly right. The new "typically … a foreign key to a specific part is still legal and common in practice, but often unnecessary" framing acknowledges the pattern DJ's own tests use, and the tier-split on insertion atomicity (make()-enforced for Computed/Imported vs convention for Manual) correctly separates the two cases while keeping the deletion-cascade universality intact. Approving.
Manual tables are the tier whose rows are inserted directly from outside the pipeline — whether entered by a person (form/GUI) or loaded by an automated ingestion tool — not 'entered by hand'. The defining property is direct insertion vs. production by make(), not human authorship.
|
Additional fix pushed (aa4a1bc): corrected the Manual-tier definition in |
Convert transaction examples and mentions in this PR's pages from the global dj.conn().transaction to schema.connection.transaction, matching the docs-wide sweep (#231), so the pages read consistently.
Resolve autopopulate.md §5.5 conflict: keep this branch's fix (no explicit transaction inside make(); populate() already wraps one), matching the section 'No Manual Transactions Inside make()'.
Data-operation how-to and spec improvements from the book → docs coverage review. Closes #221, #222, #223, #224, and adds the delete → garbage-collection cross-reference.
What's here
insert-data.md— "What Not to Insert" (insert-data how-to: add insertion-discipline / reproducibility guardrails #221): the how-to now warns that the examples target Manual tables and you must not hand-insert into Computed/Imported (rows come frommake()/populate()), Lookup rows come fromcontentsin the definition, insert parents before children, and never open a transaction insidemake().update-data.md— new how-to (Add an 'update data' how-to #223): updates are rare and surgical; prefer delete + reinsert (+ recompute);update1()for single-row corrections; primary keys and computed results can't be updated.master-part.mdspec (master-part spec: state that a dependency on the master implies its parts #222): adds transitive completeness — a dependency on the master is a dependency on all of its parts.autopopulate.mdspec §5.5 fix (Fix contradiction: autopopulate.md §5.5 shows a transaction inside make() #224): it showed a transaction opened insidemake(), contradictingcomputation-model.md;make()is already transactional (a nested one raises). Replaced with correct guidance.delete-data.md: "Reclaiming Object Storage" note + link to the garbage-collection how-to (deletes leave in-store objects; GC reclaims them).Draft for review on the docs server. No dependency on the v2.3.x code work.