Add RowArrayCursor fixed-width prefetch tests and Codecov CI job#518
Open
Yaraslaut wants to merge 4 commits into
Open
Add RowArrayCursor fixed-width prefetch tests and Codecov CI job#518Yaraslaut wants to merge 4 commits into
Yaraslaut wants to merge 4 commits into
Conversation
CHAR(N)/NCHAR(N) columns are excluded from the transparent automatic prefetch gate and always fall back to per-row SQLGetData there, but RowArrayCursor (the underlying bulk block-fetch mechanism reachable via ExecuteBatchFetch) binds and reads them directly. Add coverage for that path: right-padding, NULL vs empty-string, a value that exactly fills its declared width, and a single-block (no-continuation) fetch. Also add a "coverage" CI job mirroring the existing "Sanitizers" job's structure (clang-coverage preset, sqlite3-only, no Docker services) and upload the resulting lcov report to Codecov via codecov-action@v5. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
apt.llvm.org's installer (llvm.sh) only ever installs version-suffixed binaries (llvm-cov-22), never a bare llvm-cov symlink. The unversioned find_program(LLVM_COV_PATH llvm-cov) lookup silently failed on the new coverage CI job, so lcov fell back to the system gcov (from GCC), which cannot parse Clang's gcov-compatible data format: geninfo: ERROR: Incompatible GCC/GCOV version found while processing .../SqlLogger.cpp.gcda Search for the version matching the active compiler first, then a few recent majors, then the unversioned name (covers Homebrew LLVM on macOS, which does install a bare llvm-cov). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
SQLite3's ODBC driver reports CHAR(N)/NCHAR(N) as Char vs WChar inconsistently across platforms (confirmed: WChar for both on Windows, but Char for NCHAR(8) on Linux) — reproduced locally via Docker with the exact CI toolchain (Ubuntu 24.04, Clang 22, clang-asan-ubsan preset) after Sanitizers/macOS jobs failed on this branch while passing on master. No sanitizer defect found; RowArrayCursor normalizes both bound types to UTF-8 correctly either way, so relax the assertion to accept either, matching the pattern already used for the narrow column. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace direct cursor.GetI64(...).value() / GetString(...).value() calls with RequireI64/RequireString helpers that REQUIRE + explicit if-throw, mirroring the existing RequireParsed pattern in SqlGuidTests.cpp — the analysis can trace an explicit if/throw as a null check but not a Catch2 REQUIRE macro. Reproduced and verified locally against the exact CI toolchain (Ubuntu 24.04, Clang 22, clang-debug preset + run-clang-tidy) via Docker, since this job only runs on non-master branches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds new test coverage for RowArrayCursor’s bulk (block) fetch behavior with fixed-width CHAR(N)/NCHAR(N) columns—covering right-padding behavior, NULL vs empty string, exact-width values, and single-block fetches—and introduces a CI coverage job that generates an lcov report and uploads it to Codecov.
Changes:
- Add
RowArrayCursorfixed-widthCHAR/NCHARprefetch tests (padding trim, NULL/empty, full-capacity, single-block). - Improve Clang coverage tool discovery in CMake by preferring version-suffixed
llvm-cov-*binaries. - Add a GitHub Actions “coverage” job that builds with the
clang-coveragepreset, runs thecoveragetarget, and uploads lcov output to Codecov.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/tests/SqlStatementPrefetchTests.cpp |
Adds multiple RowArrayCursor fixed-width text bulk-fetch regression tests and small local helpers. |
cmake/Coverage.cmake |
Makes llvm-cov discovery more robust for Clang + apt.llvm.org installs (version-suffixed binaries). |
.github/workflows/build.yml |
Adds a coverage CI job (sqlite3-only scope) and uploads the generated lcov file to Codecov. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+143
to
+147
| auto const value = cursor.GetI64(row, column); | ||
| REQUIRE(value.has_value()); | ||
| if (!value.has_value()) | ||
| throw std::runtime_error("REQUIRE failed but flow continued"); // unreachable | ||
| return *value; |
Comment on lines
+151
to
+156
| { | ||
| auto value = cursor.GetString(row, column); | ||
| REQUIRE(value.has_value()); | ||
| if (!value.has_value()) | ||
| throw std::runtime_error("REQUIRE failed but flow continued"); // unreachable | ||
| return std::move(*value); |
Comment on lines
+675
to
+676
| REQUIRE(actual.size() == rowCount); | ||
| CHECK(actual == expected); // byte-identical to the trusted per-row path, including NULLs and padding |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CHAR(N)/NCHAR(N) columns are excluded from the transparent automatic prefetch gate and always fall back to per-row SQLGetData there, but RowArrayCursor (the underlying bulk block-fetch mechanism reachable via ExecuteBatchFetch) binds and reads them directly. Add coverage for that path: right-padding, NULL vs empty-string, a value that exactly fills its declared width, and a single-block (no-continuation) fetch.
Also add a "coverage" CI job mirroring the existing "Sanitizers" job's structure (clang-coverage preset, sqlite3-only, no Docker services) and upload the resulting lcov report to Codecov via codecov-action@v5.