Skip to content

Conversation

@jahnvi480
Copy link
Contributor

@jahnvi480 jahnvi480 commented Jun 11, 2025

AB#37746
This pull request introduces several enhancements to the database interaction logic and improves the testing framework for SQL data types. The most significant changes include adding support for named tuples in fetchone results, improving error handling in the C++ bindings, and refactoring test cases for better cleanup and assertion practices.

Enhancements to fetchone functionality:

  • mssql_python/cursor.py: Modified the fetchone method to return a named tuple when valid field names are available, enabling attribute-based access to query results. Falls back to a regular tuple if named tuple creation fails.
  • mssql_python/cursor.py: Added collections import to support named tuple creation.

Improvements to C++ bindings:

  • mssql_python/pybind/ddbc_bindings.cpp: Updated FetchOne_wrap to handle named tuples on the Python side, added error handling for column count retrieval, and improved logging for debugging.

Refactoring and cleanup in test cases:

  • tests/test_004_cursor.py: Refactored multiple test cases to use the drop_table_if_exists helper function for cleanup, ensuring tables are dropped before and after tests. [1] [2] [3]
  • tests/test_004_cursor.py: Replaced direct tuple comparisons in assertions with the compare_row_value helper function for improved readability and flexibility. [1] [2] [3]

Debugging and logging enhancements:

  • main.py: Added debug statements to print the type and content of rows, and safely handle cases where rows is None.

Copilot AI review requested due to automatic review settings June 11, 2025 06:23
Copy link
Contributor

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 PR improves database query result handling by introducing named tuple support in fetchone(), enhancing error handling in the C++ bindings, and refactoring the test framework for better maintainability.

  • Enhanced fetchone() to support attribute access via named tuples.
  • Improved error and logging behavior in the C++ bindings.
  • Refactored test cases to use a helper function for table cleanup and row value comparisons.

Reviewed Changes

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

File Description
tests/test_004_cursor.py Refactored tests to use drop_table_if_exists and compare_row_value for clearer assertions and cleanup.
mssql_python/pybind/ddbc_bindings.cpp Updated FetchOne_wrap with better error handling and corrected docstring for parameter usage.
mssql_python/cursor.py Modified fetchone to return a namedtuple when possible, enabling both index and attribute access.
main.py Added debug prints for row type and content to help trace fetchone results.
Comments suppressed due to low confidence (1)

mssql_python/pybind/ddbc_bindings.cpp:1810

  • The parameter name in this docstring should be updated to 'row_list' to match the function signature for clarity.
// @param row: A Python object reference that will be populated with a named tuple containing the fetched row data.

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.

The PR follows good practices, is secure, and robustly upgrades the user-facing API. With minor refactoring for efficiency and logging, it would be even better. No memory leaks, deadlocks, or security issues identified. Logic is correct, and changes are well-tested.

sumitmsft
sumitmsft previously approved these changes Jun 16, 2025
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.

As discussed, it would be cleaner and more future-proof to define a dedicated Row class instead of relying on namedtuple generation at runtime. By having a separate Row class, we can:

Encapsulate row behavior: A custom class allows us to centralize logic for attribute and index access, custom formatting, and future enhancements (like dict-like access, type conversions, or richer error handling).
Improve maintainability: Making changes or adding features to row handling (e.g., supporting column name mapping, case-insensitivity, or value transformations) becomes much easier in a single class, without having to manage the quirks of namedtuple limitations.
Enable richer row features: A dedicated class can provide context-aware methods, like .as_dict(), or even hooks for validation, which would be awkward with namedtuples.

Could we refactor this so that fetchone, fetchmany, and fetchall return instances of a Row class, instead of namedtuples or plain tuples? This would keep the codebase cleaner and also pave the way for any future features related to row handling.
Also, the previous tests should be working fine with any introduction of Row related changes.
We should also add new tests for access row data using column name etc.

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.

Requesting some changes w.r.t tests.
Change the PR title to (Row class implementation)

@jahnvi480 jahnvi480 changed the title FIX: Allowing access using column name and index FIX: Row Object support Jun 17, 2025
@bewithgaurav bewithgaurav merged commit 54b649b into main Jun 18, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants