SQLFetch and SQLGetData update - column wise fetch and incremental PLP support - #153
SQLFetch and SQLGetData update - column wise fetch and incremental PLP support#153shiwanigupta0809 wants to merge 17 commits into
Conversation
Wire pause_before_first_column in receive_row_into_internal; drain stale paused rows on advance in get_next_row_into; add is_unicode_text to ColumnMetadata; convert UTF-16LE to UTF-8 in stream_active_plp_chunk when target is SQL_C_CHAR to fix infinite loop and truncated NVARCHAR reads.
1. Backward/re-read enforcement (07009 + SQL_NO_DATA): add current_row_last_col to StmtState; reject col < last with 07009 and col == last (already consumed) with SQL_NO_DATA; reset on each SQLFetch. Also fixes PlpVarcharMaxStreamed returning SQL_ERROR instead of SQL_NO_DATA after stream exhaustion. 2. TwoPlpColumnsStreamedWithSkippedPlpBetween panic: OdbcRowWriter set_column now fills any gap left by a streamed PLP column with Null placeholders instead of debug_assert-panicking on the non-sequential index. 3. VarbinaryMaxToCharReturnsHyc00: add is_binary_type() to ColumnMetadata; track active_plp_is_binary in StmtState; reject SQL_C_CHAR / SQL_C_WCHAR target for binary PLP columns (HYC00). 4. PlpSmallValueIndicatorDecrements: driver correctly returns SQL_NO_TOTAL (spec-compliant); relax the test to only verify total byte count, not specific indicator values.
37afb82 to
8b07ce2
Compare
|
@copilot resolve the merge conflicts in this pull request |
|
@copilot resolve the merge conflicts in this pull request |
Resolved in commit a9c2da4 by merging
Resolved in commit a9c2da4 by merging |
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql-odbc/src/api/close_cursor.rsmssql-odbc/src/api/fetch.rsmssql-odbc/src/api/get_data.rsmssql-odbc/src/api/more_results.rsmssql-odbc/src/row/row_writer.rsmssql-tds/src/io/token_stream.rsmssql-tds/src/query/metadata.rs🔗 Quick Links |
a9c2da4 to
a3efdc4
Compare
Remove from_row() and the Vec pre-population dance. set_column() now only stores the single target column (where pause_after_column fires); all other columns are decoded by the TDS layer and discarded. col_in_row detection switches from row.len() >= col_index to !row.is_empty(), and write_column_as_text uses row.first() instead of row[col_index - 1]. resume_row_to_column no longer takes/restores current_row since it no longer needs the old column data.
…reset_row_stream
OdbcRowWriter: drop Vec<ColumnValues> entirely; use requested_col/captured/
end_row_fired; pause_before_first_column() always returns true (ODBC invariant,
not a flag); pause_after_column uses 0-based col directly (no col+1 offset).
Callers use request(0-based), take_captured(), end_row_fired().
PlpEncoding {Utf16Text, SingleByteText, Binary}: replaces the two booleans
active_plp_is_unicode + active_plp_is_binary in StmtState with a single
exhaustive enum that also covers JSON (SingleByteText).
reset_row_stream(): new StmtState helper that clears all row/PLP state in one
call. Replaces five-line scattered clears in close_cursor, exec_direct,
execute, more_results, prepare, and fetch.
pause_before_first_column() now returns true only when no column is requested (position/drain mode used by SQLFetch), and false when a column is requested (resume mode used by SQLGetData). This prevents get_next_row_into from draining the current row and advancing to the next one when SQLGetData calls resume_row_to_column.
…w target-type switch
- Replace active_plp_column / active_plp_target_type / active_plp_encoding
with Option<ActivePlpStream { column, encoding }> in StmtState; the three
scattered fields collapse into one cohesive struct.
- Remove the mid-stream target-type consistency check: msodbcsql allows the
caller to switch SQL_C_CHAR / SQL_C_WCHAR between repeated SQLGetData calls
on the same PLP column, so target_type is dropped from ActivePlpStream.
- Split row/mod.rs into row/mod.rs (re-export only) + row/row_writer.rs
(OdbcRowWriter + PlpEncoding), matching the pattern used by other modules.
02c97b8 to
8e85275
Compare
StmtState had an overloaded Option<Vec<ColumnValues>>: - Some([]) meant "row positioned" (set by SQLFetch) - Some([v]) meant "captured value" (used only in now-removed unit tests) The dual-use made the code hard to follow and kept a dead code path alive in sql_get_data_safe (the bottom write_column_as_text call was never reached in production since row.len() was always 0). Replace with two purpose-built fields: - row_positioned: bool - set by SQLFetch, cleared by reset_row_stream - last_captured: Option<ColumnValues> - set by resume_row_to_column The dead bottom write_column_as_text call is removed. The 9 unit tests that directly injected current_row are removed.
write_column_as_text was consuming last_captured before checking target_type, so HYC00 silently dropped the value. A follow-up SQLGetData on the same column then re-entered resume_row_to_column on an already-exhausted TDS position and hit SQLSTATE 24000. Fix: - Check target_type before last_captured.take() so HYC00 leaves the captured value intact for retry. - Carry the 1-based column index alongside the value in last_captured (now Option<(usize, ColumnValues)>) so a retry on the same column short-circuits the resume entirely.
|
@copilot resolve the merge conflicts in this pull request |
1 similar comment
|
@copilot resolve the merge conflicts in this pull request |
|
@copilot resolve the merge conflicts in this pull request |
…etdataUpdate # Conflicts: # mssql-odbc/src/api/exec_direct.rs # mssql-odbc/src/api/execute.rs # mssql-odbc/src/api/more_results.rs # mssql-odbc/src/handles/stmt.rs # mssql-odbc/tests/e2e/CMakeLists.txt
Resolved in commit eb929f9 by merging
Resolved in commit eb929f9 by merging
|
There was a problem hiding this comment.
Pull request overview
Adds deferred column decoding and incremental PLP streaming for ODBC SQLFetch/SQLGetData.
Changes:
- Adds pause-before-column row decoding.
- Streams requested PLP data across calls.
- Adds SQLGetData unit and end-to-end coverage.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
mssql-tds/src/query/metadata.rs |
Adds text/binary type classification. |
mssql-tds/src/io/token_stream.rs |
Pauses before decoding columns. |
mssql-tds/src/datatypes/row_writer.rs |
Adds initial-pause hook. |
mssql-tds/src/connection/tds_client.rs |
Drains paused rows when advancing. |
mssql-odbc/tests/e2e/tests/get_data_test.cpp |
Adds SQLGetData scenarios. |
mssql-odbc/tests/e2e/CMakeLists.txt |
Registers the new tests. |
mssql-odbc/src/row/row_writer.rs |
Implements the ODBC row writer. |
mssql-odbc/src/row/mod.rs |
Exposes row-writing components. |
mssql-odbc/src/lib.rs |
Registers the row module. |
mssql-odbc/src/handles/stmt.rs |
Adds row and PLP continuation state. |
mssql-odbc/src/api/prepare.rs |
Resets streaming state on prepare. |
mssql-odbc/src/api/odbc_types.rs |
Documents SQL_NO_TOTAL. |
mssql-odbc/src/api/more_results.rs |
Resets state between result sets. |
mssql-odbc/src/api/get_data.rs |
Implements request-driven retrieval. |
mssql-odbc/src/api/fetch.rs |
Defers row materialization. |
mssql-odbc/src/api/execute.rs |
Resets state before execution. |
mssql-odbc/src/api/exec_direct.rs |
Resets state before direct execution. |
mssql-odbc/src/api/close_cursor.rs |
Clears streaming state on close. |
mssql-odbc/Cargo.toml |
Adds encoding dependencies. |
Comments suppressed due to low confidence (5)
mssql-odbc/src/api/get_data.rs:548
SQL_C_WCHARis handled as if every PLP source were UTF-16LE. ForVARCHAR(MAX)/JSON,payloadcontains single-byte, collation-encoded data, so grouping pairs withu16::from_le_bytesturns ordinary text into unrelated code points (for example,ABbecomes U+4241). Decode the source according toPlpEncodingand the active collation before producing UTF-16 output.
if target_type == SQL_C_WCHAR {
let usable = read & !1;
let units: Vec<u16> = payload[..usable]
.chunks_exact(2)
.map(|pair| u16::from_le_bytes([pair[0], pair[1]]))
.collect();
mssql-odbc/src/api/get_data.rs:568
- This conversion consumes the entire UTF-16 wire chunk before knowing how many UTF-8 bytes fit. An odd read drops one byte via
read & !1, and non-ASCII text can expand beyond the output buffer, causingcopy_with_nulto discard converted bytes that cannot be recovered on the next call (and potentially split a UTF-8 sequence). Preserve decoder and pending-output state across calls, and only consume source bytes whose converted output can be retained.
} else if is_unicode_plp {
// NVARCHAR PLP wire bytes are UTF-16LE; convert to UTF-8 for SQL_C_CHAR.
let usable = read & !1;
let units: Vec<u16> = payload[..usable]
.chunks_exact(2)
.map(|b| u16::from_le_bytes([b[0], b[1]]))
.collect();
let utf8 = String::from_utf16_lossy(&units);
let utf8_bytes = utf8.as_bytes();
unsafe {
copy_with_nul(
target_value_ptr as *mut u8,
buffer_length as usize,
utf8_bytes,
);
mssql-odbc/src/api/get_data.rs:530
- If the DBC mutex is poisoned here, the client is dropped and the function still finalizes the PLP call as success or success-with-info. This loses the connection while telling the application its read succeeded. Return
SQL_ERRORwhen the client cannot be restored.
if let Ok(mut dbc_state) = dbc.inner.lock() {
dbc_state.client = Some(client);
dbc_state.active_stmt = Some(statement_handle);
}
mssql-odbc/tests/e2e/tests/get_data_test.cpp:572
- This expected HYC00 is explicitly mssql-odbc-specific; msodbcsql supports more binary conversions, so the parity run should not execute this assertion. Add
SKIP_IF_COMPARING_MSODBCSQL()at the start of the test, following the established Phase-1 test pattern.
TEST_F(GetDataLiveTest, VarbinaryMaxToCharReturnsHyc00) {
ASSERT_SQL_OK(ExecDirect("SELECT CAST(0x41424344 AS VARBINARY(MAX)) AS c1"),
mssql-odbc/src/api/get_data.rs:578
- The single-byte PLP path copies server code-page bytes directly, unlike non-PLP strings, which
SqlString::to_utf8_stringdecodes using the column collation. ThusVARCHAR(MAX)andVARCHAR(n)return differentSQL_C_CHARbytes for the same non-ASCII text. Useclient.active_plp_collation()with an incremental decoder (and retain expanded pending output) before writing the application buffer.
} else {
unsafe {
copy_with_nul(
target_value_ptr as *mut u8,
buffer_length as usize,
&payload[..read],
);
write_if_some(strlen_or_ind_ptr, read as SqlLen);
| if let Ok(mut dbc_state) = dbc.inner.lock() { | ||
| dbc_state.client = Some(client); | ||
| dbc_state.active_stmt = Some(statement_handle); | ||
| } |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reject the untested varchar->SQL_C_WCHAR and binary->char PLP combinations with HYC00, and restore the SQL_NULL_DATA/SQL_DATA_AT_EXEC/SQL_NO_TOTAL constant ordering. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Treat DBC mutex poison as SQL_ERROR after row resume and PLP read instead of silently continuing and reporting a successful read on a disconnected handle. - Classify UDT as a binary PLP type in is_binary_type() so UDT payloads take the binary-type rejection path rather than being exposed as text. - Mark the two mssql-odbc-specific HYC00 SQLGetData tests (varbinary->char, varchar->SQL_C_SSHORT) with SKIP_IF_COMPARING_MSODBCSQL() so they don't fail the msodbcsql comparison leg. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
5d51ba2 to
d98f00b
Compare
Description
Aligns mssql-odbc SQLFetch and SQLGetData with ODBC's sparse, request-driven row model, and implements correct PLP (partially-length-prefixed) continuation semantics. Row and column data are now decoded lazily, on demand, rather than materializing the whole row up front.
What changed
Deferred row materialization — SQLFetch now pauses before decoding the first column. A fetched row is positioned but not read until the application requests a specific column.
Column-wise decode in SQLGetData — resumes decoding forward to the requested column instead of relying on a fully materialized row or value. Intervening unread columns are drained transparently.
Direct PLP streaming — large PLP values are streamed straight from the active TDS PLP stream across successive SQLGetData calls, with no ODBC-side cache of the fully decoded value. This keeps memory bounded regardless of value size.
Simplified continuation state — per-statement PLP tracking is reduced to the active column plus its target C type.
Tests
Added SQLGetData e2e coverage and wired it into the e2e CMake target:
Repeated small-buffer SQLGetData calls that stream a PLP value in chunks.
Mixed PLP and non-PLP column access within a row.
Sparse / non-sequential column access patterns.
Related Issues
https://sqlclientdrivers.visualstudio.com/mssql-rs/_workitems/edit/46641
Checklist
cargo bfmtpassescargo bclippypassescargo btestpasses