docs: id-first consistency — README read-back + hard_delete, sql-client-spec example - #60
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| | `primary_key` | ✅ | Drives merge/upsert and insert-only de-duplication | | ||
| | `merge_key` | ❌ | Use `primary_key` | | ||
| | `hard_delete` | ❌ | Deletes are not propagated | | ||
| | `hard_delete` | ✅ | On `merge`, rows flagged by the hint column are deleted by key while the rest upsert; requires a `primary_key`. Boolean hint deletes on `True`, other types on non-null | |
There was a problem hiding this comment.
super nit: (not blocking) The primary_key, and the without-key behaviour is silent: job_client.py:314-318 routes keyless merge through _combine_and_replace(..., apply_hard_delete=True), which drops the flagged rows from the load without deleting the matching rows already in the table, and logs nothing. Consider primary_key doesn't read "✅" and assume deletes propagated.
| ## Read your data back | ||
|
|
||
| The **same `pipeline` object** that writes can read, through dlt's standard [dataset interface](https://dlthub.com/docs/general-usage/dataset-access/dataset) — no Hotdata-specific code, no database IDs, no hand-written SQL: | ||
| Reads address the managed database by **id** (names aren't identifiers), so point the reading pipeline at the `database_id` you pinned after the first load (see [Quickstart](#quickstart)). Then dlt's standard [dataset interface](https://dlthub.com/docs/general-usage/dataset-access/dataset) reads it back — no Hotdata-specific code, no hand-written SQL: |
There was a problem hiding this comment.
super nit: (not blocking) Same claim-audit target as the ones this PR fixes, just above the diff: the tagline on line 9 ("then read it back through the same pipeline") and the highlight on line 17 still describe the read as happening through the same pipeline. That holds only when database_id was pinned on the writing pipeline — which the Quickstart deliberately doesn't do on first run, which is exactly why this section now constructs a second pipeline. A short qualifier on line 9 would keep the intro and this section consistent.
Keyless merge falls back to _dlt_id, which a source's delete row won't match, so without a primary_key flagged rows are dropped from the batch but existing rows aren't deleted (silently). ✅ overstated it — use⚠️ with the no-key caveat. Addresses claude[bot]'s review on PR #60.
| ## Read your data back | ||
|
|
||
| The **same `pipeline` object** that writes can read, through dlt's standard [dataset interface](https://dlthub.com/docs/general-usage/dataset-access/dataset) — no Hotdata-specific code, no database IDs, no hand-written SQL: | ||
| Reads address the managed database by **id** (names aren't identifiers), so point the reading pipeline at the `database_id` you pinned after the first load (see [Quickstart](#quickstart)). Then dlt's standard [dataset interface](https://dlthub.com/docs/general-usage/dataset-access/dataset) reads it back — no Hotdata-specific code, no hand-written SQL: |
There was a problem hiding this comment.
super nit: (not blocking) Anchored here because it's the nearest diff line — the actual spot is Verify a load, lines 266–277. Those CLI examples still address the database by name (hotdata databases tables list --database sales, hotdata query ... -d sales), which sits awkwardly next to line 96 ("names are not unique, so a name can't identify one") and next to this section, which now tells readers to address reads by id. The Quickstart actively produces the ambiguous case: an unpinned second run creates another database labelled sales, so -d sales then has two candidates and the verify step may inspect the wrong one.
If the CLI takes an id, reusing the db_abc123 printed on first-run create would put the whole README on one addressing story; if it's name-only, a one-line "the CLI resolves by label — pin the id if you have duplicates" covers it. Out of this PR's diff, so entirely your call.
There was a problem hiding this comment.
Both corrections check out against the code: reads resolve id-first (hotdata_client.py:119 raises no managed database resolved for this run with no pinned id), and the hard_delete job_client.py:296-310), keyless merge falls back to _dlt_id in combine_tables (merge.py:149-153) so flagged rows are dropped without propagating deletes. One out-of-diff super nit inline, non-blocking.
rohan-hotdata
changed the title
rohan-hotdata
changed the title
| # NOTE (id-first, 0.11.0): reads resolve the managed DB by id. In a single | ||
| # process the read needs a database_id — pin the id logged on the first-run | ||
| # create above via hotdata(database_id="db_..."). Without it the read raises | ||
| # "no managed database resolved". See README "Read your data back". |
There was a problem hiding this comment.
nit: (not blocking) The Acceptance line 9 lines below (line 471) is the claim this note contradicts, and the sweep left it: "step 2 prints DataFrames matching what step 1 wrote — with no database IDs, no -d flag, no CLI — proving the same pipeline object writes and reads."
The note is correct — execute_sql goes through _require_db(), and the resolved record is cached on the HotdataClientConfiguration instance (_hotdata_db, hotdata_client.py:72-79), which a later pipeline.dataset() doesn't share because dlt builds a fresh config per client. So the read does need the pinned id even in one process, and "no database IDs" is exactly the stale claim this PR is auditing for. Worth striking the "no database IDs" clause (keep "no -d flag, no CLI, no hand-written SQL") so the acceptance criterion and the note directly above it agree.
There was a problem hiding this comment.
nit: (not blocking) Scope mismatch in the metadata, not the diff: the title says spec/handover/dep-compat and commit 63aa527's message describes edits to docs/sql-client-handover.md and docs/dependency-compatibility.md, but the PR changes only README.md and docs/sql-client-spec.md — neither of those files exists in the repo (docs/ holds architecture.md, runbook.md, sql-client-spec.md). Either those edits belong to a different repo and got described here by mistake, or they were dropped. Worth trimming the title/commit body before merge so the history doesn't record a sweep that didn't happen.
The two content corrections check out: keyless merge routes through _combine_and_replace(..., apply_hard_delete=True) with fallback_key="_dlt_id", so flagged rows are dropped from the batch and existing rows survive — the job_client.py:289-303) and the boolean-vs-non-null rule (merge.py:56-63) match too.
Addresses two README review nits on PR #60: - intro tagline said 'read it back through the same pipeline' — reads need the pinned database_id, so reword to dlt's dataset interface. - 'Verify a load' addressed the DB by name (-d sales); unpinned reruns can mint duplicate 'sales' DBs, making the name ambiguous. Use the id (db_abc123, as printed on first-run create) so the whole README addresses by id.
| # NOTE (id-first, 0.11.0): reads resolve the managed DB by id. In a single | ||
| # process the read needs a database_id — pin the id logged on the first-run | ||
| # create above via hotdata(database_id="db_..."). Without it the read raises | ||
| # "no managed database resolved". See README "Read your data back". |
There was a problem hiding this comment.
nit: (not blocking) This note is correct, but the Acceptance line 9 lines below (line 471) still asserts the opposite: "step 2 prints DataFrames matching what step 1 wrote — with no database IDs, no -d flag, no CLI — proving the same pipeline object writes and reads." As of 0.11.0 the read goes through _require_db() (hotdata_client.py:104-118), which raises KeyError("no managed database resolved for this run (set database_id)") unless an id is configured — and tests/test_e2e_inmemory.py:_dest pins database_id for every pipe.dataset() test, so there's no in-process path that reads without one.
Leaving the acceptance criterion as-is makes this doc contradict itself within ten lines, which is the exact class of stale claim the PR is auditing for. Suggest striking just the "no database IDs" clause and keeping the rest.
|
|
||
| # 2) READ (unlocked by this spec) | ||
| # NOTE (id-first, 0.11.0): reads resolve the managed DB by id. In a single | ||
| # process the read needs a database_id — pin the id logged on the first-run |
There was a problem hiding this comment.
super nit: (not blocking) Two more name-based leftovers in this same file that the sweep didn't reach — flagging together since the commit describes a "full claim-audit of docs/":
- line 260: "right after resolving name→id" — there is no name→id resolution left;
_bind_by_idfetchesGET /databases/{id}. - lines 485–486 (the §15 flow diagram):
client.execute_sql(sql, database="roundtrip_demo")/(resolve name→id, submit, poll, fetch Arrow).
Either is a one-word fix, or an archival banner on the doc as a whole would cover them along with the example on line 451 that still constructs hotdata(database_name=..., ...) with no id.
Separately: the commit message for 63aa527 ("docs: id-first consistency sweep across docs/") describes edits to docs/sql-client-handover.md (archival banner) and docs/dependency-compatibility.md (struck roadmap note), but neither file exists in docs/ (only architecture.md, runbook.md, sql-client-spec.md) and neither appears in the diff — the commit only touched this file. Worth amending the message before merge so the history of a claim-audit PR doesn't itself record two changes that never happened.
21e5eab to
dca3cb6
Compare
| | `primary_key` | ✅ | Drives merge/upsert and insert-only de-duplication | | ||
| | `merge_key` | ❌ | Use `primary_key` | | ||
| | `hard_delete` | ❌ | Deletes are not propagated | | ||
| | `hard_delete` | ⚠️ | **With** a `primary_key`: on `merge`, flagged rows are deleted by key while the rest upsert (boolean hint deletes on `True`, other types on non-null). **Without** a `primary_key`, flagged rows are dropped from the batch but existing matching rows are *not* deleted — deletes silently don't propagate | |
There was a problem hiding this comment.
nit: (not blocking) The "With a primary_key" half is true only for the default upsert merge strategy. With merge_strategy="insert-only" and a primary_key, job_client.py:314-318 routes to _combine_and_replace("insert-only", apply_hard_delete=True), which drops the flagged rows from the batch and leaves existing matching rows in place — same silent non-propagation as the keyless case. tests/test_job_client.py:348 (_hd_table(strategy="insert-only") sets keyed=True) and :389 both assert exactly that.
Since the strategies table two rows up lists insert-only as ✅, a user on that strategy reads this row's "with a primary_key" clause and gets the wrong answer. Suggest scoping the condition to the strategy, e.g. "On merge with a primary_key (default upsert strategy): …", and folding insert-only in with the keyless case ("flagged rows are dropped from the batch but existing rows are not deleted").
dca3cb6 to
48463a5
Compare
| # NOTE (id-first, 0.11.0): reads resolve the managed DB by id. In a single | ||
| # process the read needs a database_id — pin the id logged on the first-run | ||
| # create above via hotdata(database_id="db_..."). Without it the read raises | ||
| # "no managed database resolved". See README "Read your data back". |
There was a problem hiding this comment.
nit: (not blocking) The note is right, but the Acceptance line nine lines down (line 471–472) still reads "with no database IDs, no -d flag, no CLI — proving the same pipeline object writes and reads." So the doc now asserts both sides within ten lines, in the PR whose stated job is retiring exactly this stale claim. Striking just the "no database IDs" clause (keeping "no -d flag, no CLI") makes the acceptance criterion agree with the note directly above it.
| pipeline = dlt.pipeline( | ||
| pipeline_name="my_pipeline", | ||
| destination=hotdata(workspace_id="your_workspace_id", database_id="db_abc123"), | ||
| ) |
There was a problem hiding this comment.
super nit: (not blocking) This rebinds the same pipeline name from the Quickstart but with a slimmer destination — declared_tables (and database_name) are gone. A reader who pastes both into one script and later calls pipeline.run(...) on this object still writes to the right database (id is pinned) but has silently dropped declared_tables, which the "Multiple tables" section (line ~264) says you need when adding a table on a later run.
Showing it as the Quickstart config plus database_id would keep one pipeline definition across write and read:
| pipeline = dlt.pipeline( | |
| pipeline_name="my_pipeline", | |
| destination=hotdata(workspace_id="your_workspace_id", database_id="db_abc123"), | |
| ) | |
| pipeline = dlt.pipeline( | |
| pipeline_name="my_pipeline", | |
| destination=hotdata( | |
| workspace_id="your_workspace_id", | |
| database_id="db_abc123", # the id printed on first-run create | |
| declared_tables=["orders"], | |
| ), | |
| ) |
Bring the docs in line with 0.11.0's id-first addressing (#59): README.md - Read-back: reads resolve the managed database by id, so configure database_id on the reading pipeline (drops the stale "no database IDs" claim that now errors). - hard_delete: marked conditional (⚠️ ) — deletes propagate only on the default `upsert` merge strategy with a primary_key; keyless merge and insert-only drop flagged rows without removing existing ones. - Intro tagline and "Verify a load" now address the database by id, so the whole README tells one addressing story (avoids the ambiguous duplicate-name case). docs/sql-client-spec.md - Note that the write→read example needs a pinned database_id under id-first. Docs-only; found via a full claim-audit of the docs rather than a string search.
48463a5 to
26bb4cd
Compare
| print(info) | ||
|
|
||
| # 2) READ (unlocked by this spec) | ||
| # NOTE (id-first, 0.11.0): reads resolve the managed DB by id. In a single |
There was a problem hiding this comment.
super nit: (not blocking) Anchored on the nearest diff line — the actual spot is docs/runbook.md:43, which the sweep didn't reach:
- Build pipeline with
destination=hotdata(database_name=..., declared_tables=[...]).
Under id-first that recipe makes every run of the new pipeline create a fresh managed database — same class of stale name-addressing guidance this PR is retiring, and it contradicts the demo section eleven lines above (runbook.md:34-37), which already tells you to pass --database-id on later runs. A database_id=... in the tuple, or a trailing "(pin database_id to reuse the same database on later runs)", covers it.
Everything else checks out: the hard_delete row matches job_client.py:287-322 and merge.py:148-168 (keyed non-insert-only merge splits upsert/delete; insert-only and keyless merge drop the flagged rows without removing existing ones — tests/test_job_client.py:389,402), the read-back section matches _require_db() (hotdata_client.py:104-119), and the
There was a problem hiding this comment.
Cycle-2 re-review: prior blocking items — none were blocking; the substantive nits are addressed. hard_delete is now insert-only merge, the intro tagline no longer claims same-pipeline read-back, the read example carries declared_tables, and "Verify a load" is id-addressed (flag confirmed by the author). Verified the new claims against job_client.py, merge.py, hotdata_client.py, and tests/test_job_client.py. One out-of-diff super nit left inline.
1 participant
Summary
Two README claims went stale and slipped through when the id-first change (0.11.0, #59) updated the quickstart. Found via a full claim-audit of the README (reading for truth, not string-searching the changed terms).
Fixes
no managed database resolved for this run) because the read has no id. Rewritten to configuredatabase_idon the reading pipeline (the id printed on first-run create). (As-released wording — reads require a pinned id. If the read-after-write follow-up lands, this relaxes again.)hard_delete— was marked ❌; it's been supported since 0.10.0 (merge deletes flagged rows by key). Corrected to ✅ with theprimary_keyrequirement noted.Release note
Docs-only. Updates the GitHub README on merge; the corrected README reaches PyPI with the next version publish (the frozen
v0.11.0tag/artifact is unchanged). No release needed.