perf(sources): read_rows uses the response body when it is the whole result - #81
Conversation
…result `read_rows` always fetched the persisted result: four requests plus a readiness wait, for rows the first response had usually already returned in full. `truncated: false` is the API's guarantee that "the result fit entirely in this response", so completeness is established there and the extra round trips buy nothing. Measured against a live workspace, ~0.8s per 10,000 rows either way versus ~2.2s always taking the persisted path -- parity with a direct inline read. The persisted path is still taken when the body is a bounded preview, which is where it earns its cost: the inline cap is a BYTE budget, so a wide table can cap at a few hundred rows where a narrow one returns ten thousand, and the persisted result is not capped at all. Row counts are checked on both routes -- `total_row_count` against the body, `X-Total-Row-Count` against the result. The trade this makes deliberately: the body is JSON and the persisted result is Arrow, so a timestamp arrives as text on one route and a datetime on the other. Dicts are already lossier than Arrow and a caller asking for them has accepted that; `read_arrow` / `read_arrow_batches` always use the persisted result and are unchanged, so anything needing one stable set of types has a path that never varies. `_submit` is split into `_query` (the request) and `_resolve` (the readiness wait) so the fast path can reach the persisted result without submitting twice.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
The split of _submit into _query/_resolve is clean, and I confirmed the scoping claim in the description: sources.py:145 uses only read_arrow_batches, so the JSON-vs-Arrow type divergence really is confined to read_rows callers.
The blocking issue is that the new fast path establishes completeness by absence of a negative rather than by an affirmative check, in a module whose stated reason for existing is refusing the opposite.
Blocking Issues
-
src/hotdata_dlt_destination/source_client.py:208—not response.truncatedfails open. Atruncatedthat is absent orNonetakes the fast path, returning a capped preview as the whole result.truncatedwas added as a field in hotdata 0.4.1 (CHANGELOG:384-385), so an optional/nullable model field is the likely shape;response.truncated is Falseremoves the dependency on that question entirely. I could not inspect the SDK model —hotdatais not installed in this checkout and it is not vendored — so I am flagging the fail-open direction, not a confirmedNonein the pinned version. -
src/hotdata_dlt_destination/source_client.py:507-513— an absenttotal_row_countskips the row-count check. This compounds with (1): a response with neither field returns whatever the body held, as complete. The repo's own fixtures constructtotal_row_count: None(tests/test_source_client.py:581,:737), so the case is modelled, not hypothetical. Unlike_total_from_headers, which raisesUnverifiableReadErrorwhen it cannot verify,read_rowshas a correct route available here — the persisted result — so falling back is strictly better than trusting the body.
Action Required
- Branch on
response.truncated is False. - Fall back to the persisted result when
total_row_countis absent, instead of returning an unverified body. - Extend the tests to cover both:
truncated=Noneandtotal_row_count=Nonemust not reach the fast path.FakeSourceClientalready has the knobs for the second one.
Non-blocking comments inline: the untested positional-row branch and strict=False in _rows_from_response, the module docstring at lines 7-14/33-41 now contradicting read_rows, the result_id-vs-truncated justification in the method docstring, and the duplicated read_arrow tail.
Note on CI: CI / Test (Python 3.12) was not reported as passing when this review started (still queued or running), so I have not seen the result of the 245 tests — I have not run anything myself.
Three ways the body route could fail open, all closed: - `truncated` was tested for falsiness, so an absent or null value read as "not truncated" and a capped preview would have been returned as the whole result. Now `is False` -- the API affirmatively saying the body is everything. - An absent `total_row_count` skipped the row check entirely. Combined with the above, a response carrying no rows became a successful empty read, which under `write_disposition="replace"` truncates a destination table and reports success. `_rows_from_response` now reports `None` when it cannot verify, and `read_rows` takes the persisted result instead of trusting the boolean alone. A count that is present and disagrees still raises: the response contradicts itself and no other route resolves that. - `zip(..., strict=False)` silently dropped the tail of a short row, producing rows missing columns -- which the row count cannot detect, since it counts rows and not fields. Now `strict=True`. The positional branch was also the untested one: the SDK types `rows` as a list of lists, and a live check confirms the API sends `rows[0]` as a list, so the branch running in production was the one CI never exercised. The fake now emits the positional shape by default, with the pre-zipped form opt-in. Also extracts `_read_ready` so the fetch-check-normalise tail is not duplicated between `read_arrow` and `read_rows`, with the retry-boundary reasoning in one place, and corrects the module docstring, which still claimed every read goes to the persisted result.
There was a problem hiding this comment.
Both blocking issues from cycle 1 are fixed in code: the fast path now opens on truncated is False (source_client.py:239) and an absent total_row_count falls back to the persisted result rather than trusting the boolean alone (_rows_from_response returns None). The positional-row branch is now covered, strict=True replaces strict=False, the duplicated tail is factored into _read_ready, and the module docstring no longer claims every read goes to the persisted result.
Remaining comments are nits — two new tests do not exercise what their docstrings claim, and read_rows now returns rows for an unpersisted result where it used to raise, undocumented. Note CI's Test job had not reported when this review ran, so I have not seen the suite result.
Two of the tests added with the body route asserted nothing about it. The fallback test passed `body_total=None`, which the fake read as "use the body's own count" rather than "omit the field", so the response was fully verifiable and the rows came back via the BODY -- the assertion would have held with the fallback deleted. The null-`truncated` test asserted `None is not False`, a property of `model_construct` that holds regardless of what the source does. Both now turn on a `fetch_calls` counter, so a test says which route produced its rows instead of inferring it from a row count both routes satisfy, and the fake gained a sentinel that distinguishes "omit the count" from "count is None". Also pins the one case where `read_rows` deliberately diverges from `read_arrow`: a result that fit inline but could not be persisted has no file for the Arrow path to read, so that raises, while the body is the answer here and has been verified. The same for a verified zero -- dicts carry no schema for a destination to key off, which is the reason the Arrow path refuses it and this one does not. Stated in the docstring so the neighbouring comment arguing the other way cannot be read as covering both. Collapses two tests that duplicated earlier ones, and fixes a dedented docstring heading.
There was a problem hiding this comment.
All prior threads resolved in e11d9f8 and ce9fbfd: truncated is False, unverifiable bodies falling back rather than being trusted, strict=True on the zip, _read_ready extracted, the module docstring rewritten to match the new default path, and the route tests now asserting on fetch_calls instead of a row count both routes satisfy. The unpersisted-but-verified divergence from read_arrow is pinned by tests in both directions, including the zero-row corner.
One non-blocking test-duplication nit inline. Note CI (Test 3.12) was still in progress when this review ran, so I have not seen the suite result.
Removing two duplicates in the previous commit introduced a third: same construction and the same two assertions as the existing body-route test, which grew the `fetch_calls == 0` assertion in that commit. The counter stays falsifiable through the `== 1` assertions in the fallback tests, which is where that property actually lives.
There was a problem hiding this comment.
All prior threads resolved in code. The change since the last review is the duplicate-test deletion plus a comment noting that the fetch_calls == 1 assertions in the fallback tests are what make the == 0 assertion falsifiable — which is the right place for it.
No new findings. CI (CI / Test (Python 3.12)) had not reported passing as of this review, so the test run is unverified from here.
`update_changelog.py` only preserves the guide comment when it is the FIRST thing in the [Unreleased] body (`_GUIDE_RE` is anchored with \A). The notes for #80 and #81 were inserted above it, so `prepare` treated it as part of the notes and moved it down into the released 0.14.0 section -- leaving [Unreleased] bare and putting the authoring guide inside the payload `extract-changelog.py` hands to the GitHub Release. Moved back under [Unreleased]. Verified both ways: extracting 0.14.0 no longer carries the guide, and a simulated 0.15.0 prepare preserves it.
* chore: release v0.14.0 * chore: keep the changelog style guide under [Unreleased] `update_changelog.py` only preserves the guide comment when it is the FIRST thing in the [Unreleased] body (`_GUIDE_RE` is anchored with \A). The notes for #80 and #81 were inserted above it, so `prepare` treated it as part of the notes and moved it down into the released 0.14.0 section -- leaving [Unreleased] bare and putting the authoring guide inside the payload `extract-changelog.py` hands to the GitHub Release. Moved back under [Unreleased]. Verified both ways: extracting 0.14.0 no longer carries the guide, and a simulated 0.15.0 prepare preserves it. * chore: collapse a stray blank line left by moving the changelog guide
1 participant
read_rowsalways fetched the query's persisted result: four requests plus a readiness wait, for rows the first response had usually already returned in full.truncated: falseis the API's guarantee that "the result fit entirely in this response" — so completeness is already established there, and the extra round trips buy nothing.Measured against a live workspace
10,000-row pages, steady state (first read discarded as cold), same query and environment:
The persisted path is still taken where it earns its cost
The inline cap is a byte budget, not a row count. So the same query returns ten thousand rows against a narrow table and can cap at a few hundred against a wide one. When the body is a bounded preview,
read_rowsgoes to the persisted result, which is not capped at all — so a workspace where the cap bites gets full pages instead of a silent throughput ceiling.Completeness is checked on both routes:
total_row_countagainst the body,X-Total-Row-Countagainst the persisted result. A body that disagrees with its own count raisesIncompleteReadErrorexactly as a short persisted read does.The trade, made deliberately
The body is JSON and the persisted result is Arrow, so a timestamp arrives as text on one route and a
datetimeon the other — and which route runs depends on how large the answer happened to be.That is documented rather than hidden, and it is scoped: dicts are already a lossier representation than Arrow, and a caller asking for them has accepted that.
read_arrowandread_arrow_batchesare unchanged — they always use the persisted result, so the Arrow types they yield never vary.hotdata_tableandhotdata_queryare built on those, so the dlt resources are unaffected.If the inconsistency turns out to matter in practice, the fix is to point
read_rowsback at the persisted result — one line, at the cost of the latency above.Also
_submitis split into_query(the request) and_resolve(the readiness wait), so the fast path can reach the persisted result without submitting the query twice.245 tests pass. New coverage for both routes, for a body that contradicts its own count, and for the Arrow methods keeping the strict path.