Skip to content

Conversation

gargsaumya
Copy link
Contributor

@gargsaumya gargsaumya commented Oct 14, 2025

Work Item / Issue Reference

AB#38821

GitHub Issue: #<ISSUE_NUMBER>


Summary

This pull request adds comprehensive support for the SQL Server XML data type to the Python MSSQL driver, ensuring proper handling for insertion, retrieval, batching, and streaming of XML data. It also introduces a suite of tests to verify correct XML behavior, including edge cases like empty, large, and malformed XML values.

MSSQL Driver Enhancements

  • Added support for the SQL_SS_XML data type throughout the driver, including binding, fetching, and row size calculations, so that XML columns are handled correctly during data operations. [1] [2] [3] [4] [5]
  • Updated logic in FetchMany_wrap and FetchAll_wrap to treat XML columns as LOBs, enabling efficient streaming for large XML values. [1] [2]

Test Coverage for XML Support

  • Added multiple tests in tests/test_004_cursor.py to verify XML handling, including basic insert/fetch, empty/null values, large XML streaming, batch inserts, and error handling for malformed XML input.

@Copilot Copilot AI review requested due to automatic review settings October 14, 2025 04:34
@github-actions github-actions bot added the pr-size: medium Moderate update size label Oct 14, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request adds comprehensive support for the SQL Server XML data type to the Python MSSQL driver. The implementation includes proper handling for XML data binding, fetching, streaming, and batching operations.

  • Added SQL_SS_XML data type support throughout the driver core
  • Updated LOB handling logic to treat XML columns as streamable large objects
  • Created comprehensive test suite covering XML operations and edge cases

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
mssql_python/pybind/ddbc_bindings.cpp Added XML data type constant and integrated XML handling into column binding, fetching, and streaming operations
tests/test_004_cursor.py Added comprehensive test suite for XML functionality including basic operations, edge cases, and error handling

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

except Exception as e:
pytest.fail(f"Datetime string parameter binding test failed: {e}")
finally:
drop_table_if_exists(cursor, table_name)
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment should clarify that this XML is malformed due to mismatched tags (<item> is opened but not properly closed before </root>).

Suggested change
drop_table_if_exists(cursor, table_name)
INVALID_XML = "<root><item></root>" # malformed: <item> is opened but not closed before </root>

Copilot uses AI. Check for mistakes.

Copy link

github-actions bot commented Oct 14, 2025

📊 Code Coverage Report

🔥 Diff Coverage

100%


🎯 Overall Coverage

74%


📈 Total Lines Covered: 4190 out of 5618
📁 Project: mssql-python


Diff Coverage

Diff: main...HEAD, staged and unstaged changes

  • mssql_python/pybind/ddbc_bindings.cpp (100%)

Summary

  • Total: 10 lines
  • Missing: 0 lines
  • Coverage: 100%

📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)
mssql_python.pybind.connection.connection.cpp: 67.6%
mssql_python.ddbc_bindings.py: 68.5%
mssql_python.pybind.ddbc_bindings.cpp: 69.5%
mssql_python.pybind.connection.connection_pool.cpp: 78.9%
mssql_python.cursor.py: 79.7%
mssql_python.connection.py: 81.7%
mssql_python.helpers.py: 84.7%
mssql_python.auth.py: 85.3%
mssql_python.type.py: 86.8%
mssql_python.pooling.py: 87.5%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

@github-actions github-actions bot added pr-size: small Minimal code update and removed pr-size: medium Moderate update size labels Oct 14, 2025
break;
}
case SQL_SS_XML:
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking strictly from documentation point of view. The PR adds support for storing and retrieving XML data as a SQL Server XML datatype, treating it as a Large Object (LOB). This means:

  • If a user wishes to insert the contents of an XML file into an XML column, they must first read the file contents into a string (in Python) and then provide that string as a parameter to the driver.
  • The driver itself does not directly handle file objects or file streams as input for XML columns. It expects the XML data as a string or bytes-like object.
  • There is no evidence in the PR or the broader codebase that the driver will automatically read or stream data directly from a file object for XML columns.

This must also be documentation explicitly in the public wikis

Copy link
Contributor

@sumitmsft sumitmsft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment, otherwise LGTM

Copy link
Collaborator

@bewithgaurav bewithgaurav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in line with sumit's comment - needs some documentation only, approving from my end

#define SQL_SS_TIMESTAMPOFFSET (-155)
#define SQL_C_SS_TIMESTAMPOFFSET (0x4001)
#define MAX_DIGITS_IN_NUMERIC 64
#define SQL_SS_XML (-152)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this magic number 152 denotes here?
Pls add a comment if possible.

case SQL_SS_XML:
{
LOG("Streaming XML for column {}", i);
row.append(FetchLobColumnData(hStmt, i, SQL_C_WCHAR, true, false));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to handle (columnSize == SQL_NO_TOTAL || columnSize > 4000) clause like SQL_WLONGVARCHAR case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-size: small Minimal code update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants