Repository navigation
Conversation
8df2c62 to
d49d2dc
Compare
|
One fix before review: the backfill now puts a typed row's role word only on the chunk of the source statement that said it, not on every evidence chunk of the row. Otherwise a value from statement A could keep speaking through statement B's chunk after A is re-pushed. There's a new test for a typed row with two sources, and the description says so. |
WaylandYang
left a comment
There was a problem hiding this comment.
Thanks, the per-chunk key, the cascade, the trigger and preflight coverage, and the abort and drop handling all match what we agreed. One thing before merge: backfill step 2 copies each source's own value onto the typed row. A typed row whose two sources gave different values (S1 lines 40-52 on c1, S2 58-70 on c2, row holding 40-52) then reads back two values, so reads change, and decision 1 says they mustn't in PR 1. Please match source rows on the typed row's own value and entity (s.value IS NOT DISTINCT FROM q.value AND s.entity_id IS NOT DISTINCT FROM q.entity_id), let the rest fall through to step 3, and add a test with two sources that disagree. PR 3 will bring S2's value across. Small nit: ADR 0048 still counts 39 edges, 13 by trigger; they're now 40 and 14.
d49d2dc to
dde278f
Compare
|
Thank you for such a careful read, the two-sources case is a great catch and I'm glad you spotted it before it shipped.
I also rebased onto current dev. Migration 0121 is still free, and |
WaylandYang
left a comment
There was a problem hiding this comment.
Thanks, the backfill fix and the two-sources test are what we needed. One thing before merge: the runtime path in `materialize.rs` has the same problem. The `statement_qualifiers` copy also runs when a statement merges into an existing typed row, and the conflict key now includes the chunk. So S2 saying 58-70 on c2, merged into a row holding 40-52, leaves that row reading two values. Please copy only when the row is created, as ADR 0070 describes, and add a test. PR 3 can bring the second value across.
Two small things: `docs/design/ledger.md:33` still gives the old key, and 0070 decision 1 still says every evidence chunk, which is no longer true for typed rows.
Note for the schema version: #1466 merges first, so this PR will need 112.
…can say something new Signed-off-by: Bogdan Szabo <bogdan@re-cinq.com>
dde278f to
787a6a2
Compare
|
Thank you so much for catching the runtime path too, you were right. Here is what changed:
Thanks again for the careful reviews, they made this much better. |
Part of #1267, PR 1 of 3. Thanks for the clear answers on the issue, they made this one easy to cut.
This builds decision 1 of ADR 0070.
statement_qualifiersgets achunk_id(NOT NULL,ON DELETE CASCADE, likefact_evidence), and its key becomes(fact_id, role, chunk_id).add_statement_qualifiernow records a role word against the chunk that says it, so a re-push writes a new row for its new chunk and leaves the old one alone. The new edge is guarded the same way as the others under ADR 0048. The trigger checks that the chunk is in the fact's base, the export preflight scans it (squalifier.chunk), and the catalog guard and ADR 0048's table list it.materializecopies the new column, and copies role words only when it creates a typed row: a statement that merges into an existing row may say another value, and the row would read two. The liveness readers come in PR 2 and the re-sync in PR 3.Reads are unchanged. Migration 0121 copies each existing role word onto every evidence chunk of its fact, superseded chunks included, and
statement_qualifiers_forcollapses the copies withDISTINCT, so every read returns what it returned before. A typed or implied row takes a role word only on the chunks of the source statements that said the same value, so a value one source said never sits on another source's chunk, and a typed row whose sources disagree still reads back only its own value (PR 3 brings the other value across). A typed role no source carries with that value keeps today's value on the row's evidence. Role words on facts with no evidence, the invalidated rows thatdocuments::purgeleaves behind, are dropped, and the migration reports how many with aNOTICE. If a live fact has role words and no evidence, the migration stops, the same way 0070's audit does. The ADR's status moves to In progress, and decision 1 gets a dated revision note for typed rows.CURRENT_SCHEMA_VERSIONis 112.Tests: a role word written on two chunks makes two rows and the same chunk again adds nothing; a role word on another base's chunk is refused by the trigger and reported by the export preflight; and three scratch-database tests run migration 0121 itself: one checks the backfill and that reads still return one value, one that a typed row takes the role word only on the chunk of a source that said the same value (including a source that said 58-70 against the row's 40-52), and one that a live fact with role words and no evidence stops it.
a_bound_statement_becomes_a_typed_factmerges a statement sayingsince 2021into a row made from one saying2019, and the row keeps only2019. Each fails without its change. I ran the store, server, core and cli suites against Postgres withUTOPIA_TEST_REQUIRE_DB=1, the migrations twice, and fmt and clippy with-D warnings.Size: it adds 551 lines and removes 65, a little over 500. The migration and the test that runs it are most of that and have to ship together, so I don't see a useful split.