Skip to content

docs: id-first consistency — README read-back + hard_delete, sql-client-spec example #60

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rohan-hotdata merged 1 commit into from
Jul 24, 2026
Merged

docs: id-first consistency — README read-back + hard_delete, sql-client-spec example #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
[![dlt](https://img.shields.io/badge/dlt-1.28-blue.svg)](https://dlthub.com)
[![License: MIT](https://img.shields.io/badge/License-MIT-yellow.svg)](LICENSE)

Load data into [Hotdata](https://hotdata.dev) managed databases using [dlt](https://dlthub.com) — then read it back through the same pipeline.
Load data into [Hotdata](https://hotdata.dev) managed databases using [dlt](https://dlthub.com) — then read it back through dlt's standard dataset interface.

dlt handles extraction, schema inference, and batching. This package is a **native dlt destination** (`JobClientBase` + `WithStateSync` + `WithSqlClient`) for the Hotdata side: it uploads each batch as Parquet, registers it with your managed database, syncs pipeline state so incremental sources resume, and exposes a server-side read API.

Expand Down Expand Up @@ -98,9 +98,17 @@ Managed databases are addressed by id — Hotdata database names are not unique,

## 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:

@claude claude Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@claude claude Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


```python
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"],
),
)
Comment on lines +104 to +111

@claude claude Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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"],
),
)

ds = pipeline.dataset()

ds.table("orders").df() # whole table -> pandas.DataFrame
Expand Down Expand Up @@ -154,7 +162,7 @@ Where `hotdata` stands against the [dlt destination capability spec](https://dlt
|---------------|:-------:|-------|
| `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` with a `primary_key` (default `upsert` strategy): flagged rows are deleted by key while the rest upsert (boolean hint deletes on `True`, other types on non-null). Otherwise — keyless `merge`, or `insert-only` (even with a key) — flagged rows are dropped from the batch but existing matching rows are *not* deleted, so deletes silently don't propagate |
| `dedup_sort` | ❌ | |

### Loader file formats
Expand Down Expand Up @@ -260,14 +268,14 @@ If you add a new table later, include it in `declared_tables` on the next run
After a pipeline runs, use the [Hotdata CLI](https://github.com/hotdata-dev/sdk-python) to check that the data landed:

```bash
# List your managed databases
# List your managed databases (shows each database's id)
hotdata databases list

# Check that tables are loaded and queryable
hotdata databases tables list --database sales
# Address by id (the one printed on first-run create) — names aren't unique
hotdata databases tables list --database db_abc123

# Query the data
hotdata query "SELECT * FROM public.orders LIMIT 5" -d sales
hotdata query "SELECT * FROM public.orders LIMIT 5" -d db_abc123
Comment thread
rohan-hotdata marked this conversation as resolved.
```

## Demo pipeline
Expand Down
4 changes: 4 additions & 0 deletions docs/sql-client-spec.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,10 @@ info = pipe.run(spans())
print(info)

# 2) READ (unlocked by this spec)
# NOTE (id-first, 0.11.0): reads resolve the managed DB by id. In a single

@claude claude Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: (not blocking) Anchored on the nearest diff line — the actual spot is docs/runbook.md:43, which the sweep didn't reach:

  1. 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 ⚠️ legend on line 131 now has a user.

# process the read needs a database_id — pin the id logged on the first-run

@claude claude Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_id fetches GET /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.

# create above via hotdata(database_id="db_..."). Without it the read raises
# "no managed database resolved". See README "Read your data back".
Comment on lines +459 to +462

@claude claude Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +459 to +462

@claude claude Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@claude claude Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ds = pipe.dataset()
print(ds.table("spans").df()) # full table
print(ds("SELECT model, avg(latency_ms) AS p FROM spans GROUP BY model").df()) # aggregate
Expand Down
Loading