Skip to content

Commit 50281ed

Browse files
author
Christian Parpart
committed
[SqlStatement] Fix prefetch CI failures: cross-type string read, clang-tidy, docs, style
Address the CI matrix failures from the block-prefetch commit: - Cross-type read regression (the PostgreSQL/Windows dbtool failures): reading a prefetched numeric/temporal/GUID column as a string (GetColumn<std::string>, as dbtool's generic `exec` printer does) returned an empty string because ConvertCell only rendered character-bound cells. RenderCellAsUtf8 now formats every bound type to text (integers byte-identical to the driver; floating/ temporal/GUID via std::formatter), matching the per-row SQLGetData(SQL_C_CHAR) behaviour. Adds a [prefetch] regression test for the all-numeric-read-as-text case. - clang-tidy (-warnings-as-errors): split is moot — fixed at source. Test file: math-missing-parentheses, integer-sign-comparison, nested conditional operator, std::move on trivially-copyable fixed strings, unchecked optional access. Header: unused-lambda-capture (explicit this-> on the member call). ConvertCell was also split into per-category helpers to stay under the cognitive-complexity threshold. - Doc coverage (doxygen): @ref PrefetchDepthDefault -> @c (it is a value, not a ref target) in SqlConnection.hpp and SqlConnectInfo.hpp; drop the @param naming an unnamed parameter on SqlLogger::OnFetchBlock (described in the brief instead). - C++ style (clang-format-22): restore the single-line empty deleter lambda. Verified: clangcl-debug builds clean; [prefetch] suite green on sqlite3, mssql2022 (Docker), postgres (Docker); dbtool `exec` renders numeric columns. Signed-off-by: Christian Parpart <c.parpart@lastrada.net>
1 parent 5b66dc1 commit 50281ed

6 files changed

Lines changed: 84 additions & 24 deletions

File tree

src/Lightweight/SqlConnectInfo.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ struct [[nodiscard]] SqlConnectionDataSource
7676
/// (rows requested per @c SQLFetchScroll round-trip on the transparent per-row fetch path).
7777
///
7878
/// A value <= 1 disables prefetch (every classic loop keeps issuing one @c SQLFetch per row).
79-
/// Defaults to @ref PrefetchDepthDefault. Has effect only on backends whose driver supports
79+
/// Defaults to @c PrefetchDepthDefault. Has effect only on backends whose driver supports
8080
/// native row-array fetching (see @c SqlConnection::SupportsNativeRowArrayFetch).
8181
std::size_t defaultPrefetchDepth = PrefetchDepthDefault;
8282

src/Lightweight/SqlConnection.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,7 @@ class SqlConnection final
191191
/// instead of issuing one @c SQLFetch per row. A value <= 1 disables prefetch. Effective only when
192192
/// @ref SupportsNativeRowArrayFetch is true; otherwise statements fall back to per-row fetching.
193193
///
194-
/// @return The configured default prefetch depth (defaults to @ref PrefetchDepthDefault).
194+
/// @return The configured default prefetch depth (defaults to @c PrefetchDepthDefault).
195195
[[nodiscard]] LIGHTWEIGHT_API std::size_t DefaultPrefetchDepth() const noexcept;
196196

197197
/// @brief Sets the default block-prefetch depth for statements created on this connection.

src/Lightweight/SqlLogger.hpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,7 @@ class SqlLogger
118118
///
119119
/// Non-pure with an empty default so existing loggers need no change; override to observe how many
120120
/// network round-trips a query actually cost (N rows at depth D collapse to @c ceil(N/D) blocks).
121-
///
122-
/// @param rowsInBlock Number of rows materialized by this block fetch (0 at end of result set).
121+
/// The argument is the number of rows materialized by this block fetch (0 at end of result set).
123122
virtual void OnFetchBlock(std::size_t /*rowsInBlock*/) {}
124123

125124
/// Invoked when fetching is done.

src/Lightweight/SqlStatement.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -268,8 +268,7 @@ SqlStatement::SqlStatement(SqlConnection& relatedConnection):
268268
}
269269

270270
SqlStatement::SqlStatement(std::nullopt_t /*nullopt*/):
271-
m_data { const_cast<Data*>(&Data::NoData), [](Data* /*data*/) {
272-
} }
271+
m_data { const_cast<Data*>(&Data::NoData), [](Data* /*data*/) {} }
273272
{
274273
}
275274

src/Lightweight/SqlStatement.hpp

Lines changed: 33 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1068,7 +1068,7 @@ void SqlStatement::BindOutputColumnsToRecord(Records*... records)
10681068
((EnumerateRecordMembers(*records,
10691069
[this, &i]<size_t I, typename FieldType>(FieldType& value) {
10701070
++i;
1071-
RecordPrefetchOutputColumn<FieldType>(i, &value);
1071+
this->RecordPrefetchOutputColumn<FieldType>(i, &value);
10721072
})),
10731073
...);
10741074
return;
@@ -1798,20 +1798,42 @@ namespace detail
17981798
}
17991799
}
18001800

1801+
/// @brief Renders a block-buffer cell to UTF-8 text. Character columns are returned verbatim;
1802+
/// numeric, temporal and GUID columns are formatted to their text form. This mirrors the driver's
1803+
/// @c SQL_C_CHAR conversion on the single-row @c GetColumn path so that reading a non-character column
1804+
/// as a string (e.g. a generic "print every column as text" loop) yields the value rather than an
1805+
/// empty string. Integer text is identical to the driver's; floating/temporal text uses the value
1806+
/// type's @c std::formatter, which is backend-independent.
1807+
[[nodiscard]] inline std::string RenderCellAsUtf8(RowArrayCursor const& cursor, std::size_t row, SQLUSMALLINT column)
1808+
{
1809+
switch (cursor.ColumnBoundType(column))
1810+
{
1811+
case RowArrayCursor::BoundType::Char:
1812+
case RowArrayCursor::BoundType::WChar:
1813+
return cursor.GetString(row, column).value_or(std::string {});
1814+
case RowArrayCursor::BoundType::Int64:
1815+
return std::format("{}", cursor.GetI64(row, column).value_or(0));
1816+
case RowArrayCursor::BoundType::Double:
1817+
return std::format("{}", cursor.GetF64(row, column).value_or(0.0));
1818+
case RowArrayCursor::BoundType::Date:
1819+
return std::format("{}", cursor.GetDate(row, column).value_or(SqlDate {}));
1820+
case RowArrayCursor::BoundType::Timestamp:
1821+
return std::format("{}", cursor.GetTimestamp(row, column).value_or(SqlDateTime {}));
1822+
case RowArrayCursor::BoundType::Guid:
1823+
return std::format("{}", cursor.GetGuid(row, column).value_or(SqlGuid {}));
1824+
}
1825+
return std::string {};
1826+
}
1827+
18011828
/// @brief Reconstructs a string-like cell (plain @c std::string flavours and the Lightweight string
1802-
/// wrappers) from the block buffer. Character columns bind as @c Char (SQL_C_CHAR) or @c WChar
1803-
/// (SQL_C_WCHAR); a non-text bound type (only reachable via a cross-type read of a numeric/temporal
1804-
/// column) yields an empty value. Fixed-capacity strings get the same trailing trim the single-row
1805-
/// @c GetColumn path applies via @c SqlFixedString::PostProcessOutputColumn, so the value is
1806-
/// byte-identical; the UTF-8 bytes are then re-encoded to the target's element type.
1829+
/// wrappers) from the block buffer, rendering any bound type to text via @ref RenderCellAsUtf8.
1830+
/// Fixed-capacity strings get the same trailing trim the single-row @c GetColumn path applies via
1831+
/// @c SqlFixedString::PostProcessOutputColumn; the UTF-8 bytes are then re-encoded to the target's
1832+
/// element type.
18071833
template <typename T>
18081834
[[nodiscard]] inline T ReadStringLikeCell(RowArrayCursor const& cursor, std::size_t row, SQLUSMALLINT column)
18091835
{
1810-
using BoundType = RowArrayCursor::BoundType;
1811-
auto const boundType = cursor.ColumnBoundType(column);
1812-
if (boundType != BoundType::Char && boundType != BoundType::WChar)
1813-
return T {};
1814-
auto utf8 = cursor.GetString(row, column).value_or(std::string {});
1836+
auto utf8 = RenderCellAsUtf8(cursor, row, column);
18151837
if constexpr (SqlFixedStringCell<T>)
18161838
TrimFixedStringBytes<T::PostRetrieveOperation>(utf8);
18171839
return T { DecodeUtf8To<typename T::value_type>(utf8) };

src/tests/SqlStatementPrefetchTests.cpp

Lines changed: 47 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ SqlStatement MakePerRowStatement()
126126

127127
TEST_CASE_METHOD(SqlTestFixture, "Prefetch: raw GetColumn loop collapses round-trips", "[prefetch]")
128128
{
129-
constexpr std::size_t rowCount = 2 * TestPrefetchDepth + 7; // crosses two block boundaries
129+
constexpr std::size_t rowCount = (2 * TestPrefetchDepth) + 7; // crosses two block boundaries
130130

131131
auto stmt = SqlStatement {};
132132
stmt.Connection().SetDefaultPrefetchDepth(TestPrefetchDepth);
@@ -154,6 +154,37 @@ TEST_CASE_METHOD(SqlTestFixture, "Prefetch: raw GetColumn loop collapses round-t
154154
CHECK(probe.blockFetches < rowCount); // proves batching: far fewer round-trips than rows
155155
}
156156

157+
TEST_CASE_METHOD(SqlTestFixture, "Prefetch: numeric columns read as text render their value (cross-type)", "[prefetch]")
158+
{
159+
// A generic "print every column as a string" loop (e.g. dbtool's `exec`) reads numeric columns via
160+
// GetColumn<std::string>. On the per-row path the driver converts the value to text; the prefetch
161+
// path must do the same rather than yield an empty string. Integer text is exact; the double is only
162+
// checked for non-emptiness because its textual form is not portably defined.
163+
auto stmt = SqlStatement {};
164+
stmt.Connection().SetDefaultPrefetchDepth(TestPrefetchDepth);
165+
CreateNumericTable(stmt);
166+
constexpr std::size_t rowCount = TestPrefetchDepth + 5;
167+
FillNumericTable(stmt, rowCount);
168+
169+
PrefetchProbe probe;
170+
std::size_t seen = 0;
171+
{
172+
LoggerSwap const swap { probe };
173+
auto cursor = stmt.ExecuteDirect(std::string { kNumericSelect });
174+
while (cursor.FetchRow())
175+
{
176+
auto const rowNumber = static_cast<std::int64_t>(seen) + 1;
177+
CHECK(cursor.GetColumn<std::string>(1) == std::format("{}", rowNumber)); // Id (exact)
178+
CHECK(cursor.GetColumn<std::string>(2) == std::format("{}", rowNumber * 1000)); // Big (exact)
179+
CHECK_FALSE(cursor.GetColumn<std::string>(3).empty()); // Ratio (double)
180+
CHECK(cursor.GetColumn<std::string>(4) == std::format("{}", rowNumber % 97)); // Mid (exact)
181+
++seen;
182+
}
183+
}
184+
REQUIRE(seen == rowCount);
185+
CHECK(probe.blockFetches >= 1); // the all-numeric result set is prefetched
186+
}
187+
157188
TEST_CASE_METHOD(SqlTestFixture, "Prefetch: bound output columns with NULLs and optionals", "[prefetch]")
158189
{
159190
auto stmt = SqlStatement {};
@@ -193,8 +224,8 @@ TEST_CASE_METHOD(SqlTestFixture, "Prefetch: bound output columns with NULLs and
193224
CHECK(probe.blockFetches >= 2); // crossed at least one block boundary
194225
for (auto const i: std::views::iota(std::size_t { 0 }, rowCount))
195226
{
196-
auto const rowNumber = i + 1;
197-
CHECK(actual[i].first == static_cast<std::int64_t>(rowNumber));
227+
auto const rowNumber = static_cast<std::int64_t>(i) + 1;
228+
CHECK(actual[i].first == rowNumber);
198229
if (rowNumber % 5 == 0)
199230
CHECK_FALSE(actual[i].second.has_value());
200231
else
@@ -411,7 +442,13 @@ TEST_CASE_METHOD(SqlTestFixture, "Prefetch: string, fixed-string, wide and nulla
411442
.Set("Note", SqlWildcard));
412443
for (auto const i: std::views::iota(std::size_t { 1 }, rowCount + 1))
413444
{
414-
auto const name = i % 3 == 0 ? std::format("Grüße-{}", i) : (i % 3 == 1 ? std::string {} : std::format("row-{}", i));
445+
auto const name = [i] {
446+
if (i % 3 == 0)
447+
return std::format("Grüße-{}", i);
448+
if (i % 3 == 1)
449+
return std::string {};
450+
return std::format("row-{}", i);
451+
}();
415452
auto const note = i % 7 == 0 ? std::optional<std::string> {} : std::optional<std::string> { std::format("n{}", i) };
416453
(void) stmt.Execute(static_cast<std::int64_t>(i),
417454
name,
@@ -437,7 +474,9 @@ TEST_CASE_METHOD(SqlTestFixture, "Prefetch: string, fixed-string, wide and nulla
437474
auto wide = cursor.GetColumn<std::u16string>(3);
438475
auto tag = cursor.GetColumn<SqlTrimmedFixedString<8>>(4);
439476
auto note = cursor.GetNullableColumn<std::string>(5);
440-
rows.emplace_back(id, std::move(name), std::move(wide), std::move(tag), std::move(note));
477+
// name (SqlAnsiString<32>) and tag (SqlTrimmedFixedString<8>) are trivially copyable, so they
478+
// are passed by value; only the heap-backed wide string and optional are moved.
479+
rows.emplace_back(id, name, std::move(wide), tag, std::move(note));
441480
}
442481
return rows;
443482
};
@@ -473,8 +512,9 @@ TEST_CASE_METHOD(SqlTestFixture, "Prefetch: GUID column round-trips", "[prefetch
473512
stmt.Prepare(stmt.Query("PrefetchGuid").Insert().Set("Id", SqlWildcard).Set("Uid", SqlWildcard));
474513
for (auto const i: std::views::iota(std::size_t { 1 }, rowCount + 1))
475514
{
476-
auto const uid = SqlGuid::TryParse(std::format("12345678-1234-1234-1234-{:012}", i)).value();
477-
(void) stmt.Execute(static_cast<std::int64_t>(i), uid);
515+
auto const parsedUid = SqlGuid::TryParse(std::format("12345678-1234-1234-1234-{:012}", i));
516+
REQUIRE(parsedUid.has_value());
517+
(void) stmt.Execute(static_cast<std::int64_t>(i), *parsedUid);
478518
}
479519

480520
auto readGuids = [&](SqlStatement& source) {

0 commit comments

Comments
 (0)