Support Arrow View types in bulk copy - #259
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Arrow view-type support to mssql-py-core bulk copy, resolving Polars compatibility issue #708.
Changes:
- Maps
Utf8ViewandBinaryViewto compatible SQL types. - Adds Rust unit and live integration coverage.
- Documents the change.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
mssql-py-core/src/arrow_bulkcopy.rs |
Implements view-type planning and extraction. |
mssql-py-core/tests/test_bulkcopy_arrow_varchar.py |
Tests string_view round-tripping. |
mssql-py-core/tests/test_bulkcopy_arrow_binary.py |
Tests binary_view round-tripping. |
CHANGELOG.md |
Adds an unreleased changelog entry. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
David Engel (David-Engel)
left a comment
There was a problem hiding this comment.
Summary
Adds Arrow Utf8View/BinaryView support to mssql-py-core's Arrow bulk copy so Polars-exported C-stream columns load without a PyArrow re-encode. The change is small, correct, and well covered. No blocking findings — a couple of suggestions and nits inline.
Verified locally (worktree at 496d0c8e)
cargo fmt -- --check— cleancargo clippy --all-features --all-targets -- -D warnings(mssql-py-core) — cleancargo nextest run --lib arrow_bulkcopy— 53 passed- CI build 166711 Linux job: both new integration tests PASSED (
test_cursor_bulkcopy_arrow_varbinary_binary_view,test_cursor_bulkcopy_arrow_varchar_string_view) — confirmed not skipped arrow55.2 withfeatures = ["ffi"]does support view types over the C data interface, and CI installs unpinnedpyarrow, sopa.string_view()/pa.binary_view()are available
Correctness
The new resolve_kind arms cover NVarChar/NChar/NText, VarChar/Char/Text, VarBinary/Binary/Image, plus Xml/Json/UniqueIdentifier for Utf8View — mirroring the existing Utf8/LargeUtf8/Binary arms with no gaps I could find. NULL handling sits upstream of the match in extract_value, so view arrays get it for free, and the tests exercise both the inline (<=12 byte) and out-of-line buffer paths on each new array type.
CI
Both red checks trace to one infra flake, not this change:
- Build Stage / Build Linux ARM failed at
Wait for cross-pool SQL host endpoint— "No SQL host endpoint after 10 min on stage attempt 3." - coverage-report then timed out after 75 minutes waiting for the combined coverage artifact that the failed stage never produced.
The pipeline's own message applies here: use Rerun stage, not Rerun failed jobs — the latter won't restart the SQL host job and will time out again.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql-py-core/src/arrow_bulkcopy.rs🔗 Quick Links |
Saurabh Singh (saurabh500)
left a comment
There was a problem hiding this comment.
Adds native Arrow Utf8View and BinaryView support to Python bulk copy so Polars C-stream inputs no longer need a PyArrow table conversion. The implementation is correct and well scoped: destination mappings mirror the existing UTF-8 and binary paths, null handling is preserved, and tests cover inline and external view storage.
Verdict: Approve with comments. No blocking findings. One non-blocking unit-test assertion should be tightened; see the inline suggestion.
| match one_col_plan(DataType::Utf8View, &dest) | ||
| .extract_value(array.as_ref(), 0, &dest) | ||
| .unwrap() | ||
| { | ||
| ColumnValues::Xml(_) | ColumnValues::Json(_) => {} | ||
| other => panic!("expected Xml/Json, got {other:?}"), | ||
| } |
There was a problem hiding this comment.
This accepts either Xml or Json for both iterations, so the test still passes if the planner swaps those mappings, and it never verifies the payload. Please match the destination and returned variant together and assert the content:
| match one_col_plan(DataType::Utf8View, &dest) | |
| .extract_value(array.as_ref(), 0, &dest) | |
| .unwrap() | |
| { | |
| ColumnValues::Xml(_) | ColumnValues::Json(_) => {} | |
| other => panic!("expected Xml/Json, got {other:?}"), | |
| } | |
| let value = one_col_plan(DataType::Utf8View, &dest) | |
| .extract_value(array.as_ref(), 0, &dest) | |
| .unwrap(); | |
| match (sql_type, value) { | |
| (SqlDbType::Xml, ColumnValues::Xml(value)) => { | |
| assert_eq!(value.as_string(), text) | |
| } | |
| (SqlDbType::Json, ColumnValues::Json(value)) => { | |
| assert_eq!(value.bytes, text.as_bytes()) | |
| } | |
| (_, other) => panic!("expected {sql_type:?}, got {other:?}"), | |
| } |
Saurabh Singh (saurabh500)
left a comment
There was a problem hiding this comment.
One minor test change, ready to approve after that is incorporated
Description
Adds native Arrow
Utf8ViewandBinaryViewsupport tomssql-py-corebulk copy. String views now map to SQL character, GUID, XML, and JSON destinations, while binary views map to binary destinations without requiring producers such as Polars to re-encode their buffers.Includes Rust unit coverage, live PyArrow C-stream integration tests, and an unreleased changelog entry. The existing
mssql-py-coreversion remains 0.1.9, which is reserved on main and is not yet published to the public wheel feed.Related Issues
microsoft/mssql-python#708
https://sqlclientdrivers.visualstudio.com/mssql-python/_workitems/edit/47279
Validation
cargo nextest run --lib arrow_bulkcopy::tests(53 passed)cargo clippy --frozen --all-features --all-targets -- -D warningscargo fmt -- --checkstring_viewandbinary_view(2 passed)mssql-pythonChecklist
cargo bfmtpasses for the modified cratecargo bclippypasses for the modified cratecargo btestpasses for the full workspace