Hydrate fetched rows directly from typed SQLite columns, removing the wstring round-trip (#17)#31
Conversation
… wstring round-trip (#17) FetchRecordsQuery::GetResults stringified every fetched column into a std::wstring and materialized the whole result set as a vector<vector<wstring>> (FetchQueryResults) before Hydrate parsed each string back into the typed member. This is the performance/memory half of #17 (the correctness half - int64 truncation and REAL precision loss - was already fixed in #30). Replace the two-pass string round-trip with row-by-row direct hydration: FetchRecordsQuery now exposes StepRow() (advance the prepared statement) and HydrateCurrentRow() (read the current row straight into the destination member via sqlite3_column_int64/_double/_text+_bytes). Database::FetchRecords<T> owns the construction loop, materializing only the final vector<T> - the string double-buffer and per-value wstring allocation are both gone. FetchQueryResults, GetResults, GetColumnValue and the string-based Hydrate are removed as they're no longer needed. NULL/empty-string skip semantics are preserved exactly: a NULL column of any storage class is left unassigned, and an empty TEXT column is likewise left unassigned, matching the prior text-path behavior bit for bit (the NULL vs empty-string conflation itself is issue #20 and remains out of scope). Benchmarked FetchAll<Company>() over 100k rows against the pre-refactor build: ~2.9x faster wall-clock (225ms -> 77ms) and ~6x fewer allocations (2.4M -> 400k). Added a 50k-row round-trip test and a dedicated NULL/empty-string semantics test; full suite (62 tests) passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK
Direct hydration (previous commit) reads ints and doubles straight from the prepared statement via sqlite3_column_int64/_double, so these two string-parsing helpers no longer have any callers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f5b467716
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const auto content = reinterpret_cast<const char*>(sqlite3_column_text(stmt_, col)); | ||
| auto& v = *reinterpret_cast<std::wstring*>(GetMemberAddress(p, record, j)); | ||
| v = content; | ||
| v = StringUtilities::FromUtf8(content, byte_count); |
There was a problem hiding this comment.
When a reflected TEXT column contains a SQLite BLOB value, which can happen via UnsafeSql or an existing SQLite file despite the column affinity, this now feeds the raw blob bytes into FromUtf8. The removed GetColumnValue returned an empty string for SQLITE_BLOB and Hydrate skipped assignment, so Fetch previously tolerated those rows; invalid blob bytes now throw during fetch and prevent reading the row. The analogous DATETIME branch below has the same issue, so keep the old skip behavior for BLOBs before converting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, confirmed by reproducing it: reverting the guard reproduced exactly this - std::range_error from wstring_convert::from_bytes on invalid-UTF8 blob bytes, plus silent 0/garbage assignment on the INT/BOOL/REAL branches for the same root cause (they weren't mentioned above but have the identical issue, since the old GetColumnValue treated BLOB the same as NULL for every storage class, not just TEXT/DATETIME).
Fixed in b225f62 by extending the skip check to SQLITE_NULL || SQLITE_BLOB up front, which matches the old code's default: branch exactly for all five storage classes in one place. Added a regression test with an invalid-UTF8 blob in a TEXT-affinity column.
Generated by Claude Code
#17) The removed text-based path returned an empty string for any column whose runtime SQLite type wasn't INTEGER/FLOAT/TEXT, so both NULL and BLOB columns were skipped during Hydrate regardless of the member's declared storage class. Direct hydration only replicated the NULL half of that: a BLOB value (reachable via UnsafeSql or a foreign database file, even for a column declared as a different storage class) fell through to the typed accessors unconditionally, which could throw out of StringUtilities::FromUtf8 for invalid UTF-8 blob bytes on TEXT/DATETIME members, and silently assigned 0/0.0 instead of skipping for INT/BOOL/REAL members. Extend the skip check to SQLITE_NULL || SQLITE_BLOB, matching the old GetColumnValue's default case exactly. Adds a regression test inserting an invalid-UTF8 BLOB into a TEXT-affinity column and asserting Fetch neither throws nor corrupts the other members. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b225f624da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const auto column_count = record.member_metadata.size(); | ||
| for (size_t j = 0; j < column_count; j++) { |
There was a problem hiding this comment.
When a file-backed database already has an older version of a table with fewer columns than the current reflection (Initialize only runs CREATE TABLE IF NOT EXISTS, so it does not add new columns), SELECT * returns fewer columns than record.member_metadata.size(). The old GetResults/Hydrate path iterated the prepared statement's actual column count, but this loop now calls sqlite3_column_type/accessors for out-of-range column indexes; SQLite documents that as undefined, so fetching older rows can read invalid data or crash instead of leaving newly added members unassigned. Use the statement column count, or the minimum of it and the metadata size, when hydrating.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in cf92b99. Interesting side note from reproducing it: the bundled SQLite in this repo actually bounds-checks columnMem() internally (i<pVm->nResColumn && i>=0, else returns a NULL-typed Mem and sets SQLITE_RANGE on the connection) - so out-of-range access doesn't crash or return garbage with this vendored version. Still a real fix worth having though: it's documented UB by SQLite's own API contract (so relying on this specific build's defensive behavior is fragile against a future SQLite version bump), and it was quietly setting SQLITE_RANGE on the connection's error state on every access past the real column count.
Bound the loop with std::min(sqlite3_column_count(stmt_), record.member_metadata.size()), matching the old code's behavior exactly. Added a regression test that drops columns from an existing table to simulate the schema-drift scenario you described.
Generated by Claude Code
Database::Initialize only ever runs CREATE TABLE IF NOT EXISTS, so a table created by an older version of a reflected struct (before a field was added) is never migrated - its actual column count can be smaller than record.member_metadata.size(). The removed text-based path bounded its loop by sqlite3_column_count(stmt_) (the statement's real column count); HydrateCurrentRow used record.member_metadata.size() instead, which can index sqlite3_column_* past the end of the result row - undefined behavior per SQLite's docs, even though the vendored SQLite build happens to guard against it defensively today. Bound the loop by min(sqlite3_column_count(stmt_), member_metadata.size()), matching the old behavior exactly. Adds a regression test that drops columns from an existing table (simulating schema drift) and fetches successfully. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
Completes the performance/memory half of #17. The data-correctness half (64-bit integer truncation, REAL precision loss) was already fixed and merged in #30; this PR removes the underlying
std::wstringround-trip on the fetch path that #30 patched around.Before this change,
FetchRecordsQuery::GetResultsstringified every fetched column into astd::wstring, and materialized the entire result set asFetchQueryResults::row_values(avector<vector<wstring>>) beforeHydrateparsed each string back into the typed member. That meant every value paid an allocation + parse on top of the SQLite copy, and the whole result set was held twice (string form + typed form) before the string form could be freed.Fix
Replaced the two-pass string round-trip with row-by-row direct hydration:
FetchRecordsQuerynow exposesStepRow()(prepares the statement on first use, advances to the next row) andHydrateCurrentRow(void* p, const Reflection& record) const(reads the current row straight into the destination member viasqlite3_column_int64/sqlite3_column_double/sqlite3_column_text+sqlite3_column_bytes— no intermediate string).Database::FetchRecords<T>()(new private template) owns the row construction loop, inside the samedb_mutex_-guarded critical section as before.FetchAll<T>,Fetch<T>(predicate), andFetch<T>(id)all route through it.FetchQueryResults(header deleted),GetResults(),GetColumnValue(), the string-based staticHydrate(), andStringUtilities::ToInt/ToDouble(their only callers were the removedHydrate).Invariants preserved exactly (no observable behavior change)
sqlite3_column_int64/sqlite3_column_doubledirectly gives exact round-trips for int64 >INT32_MAXand high-precision doubles "for free" — no formatting step needed at all now.NULLcolumn of any storage class is left unassigned (member keeps its prior/default value), and a TEXT column holding a genuine empty string is likewise left unassigned — matching the old text-path behavior bit for bit. The NULL vs. empty-string conflation itself is NULL and empty string are indistinguishable, and empty values are skipped on hydrate #20 and remains untouched/out of scope.StringUtilities::FromUtf8/TimePoint::FromSystemTime; BOOL is still an== 1truth test; column↔member positional mapping (record.member_metadata[j]) is unchanged.db_mutex_critical section; the public API still returns ownedT/vector<T>values, nothing referencing the live connection escapes.Performance
Benchmarked
FetchAll<Company>()over 100k rows (in-memory DB, Release build, same machine, 3 runs each) against the pre-refactor build (#30's merged state):(Allocation counts measured via a throwaway global
operator new/deleteoverride in a standalone benchmark harness, not committed to the repo.)Test plan
DatabaseTest.FetchAllRoundTripsLargeBatchExactly— 50,000 rows with int64 values beyondINT32_MAXand high-precision doubles, fetched back and compared exactly.DatabaseTest.FetchPreservesNullAndEmptyStringSkipSemantics— raw-SQL rows with omitted (NULL) and explicit-empty TEXT columns, verifying the skip-assignment behavior is preserved.cmake --build build && ./build/tests/unit_tests).FetchQueryResults,GetColumnValue, string-basedHydrate,StringUtilities::ToInt/ToDouble) and confirmed no remaining references.Scope
This resolves #17's remaining "hydration reads typed columns directly" acceptance bullet, so #17 can be closed once this merges. Out of scope, untouched: #20 (NULL/empty-string conflation), #19's
ToInt/ToDoubleimplementation (removed entirely as dead code rather than "fixed", since nothing calls them anymore — no other issue depends on their behavior), #14 (identifier quoting), relationships/nullable fields/predicates.Generated by Claude Code