Skip to content

Conversation

@arvis108
Copy link
Contributor

Work Item / Issue Reference

GitHub Issue: #234


Summary

Summary

This PR fixes a critical issue where string parameters were being automatically converted to datetime objects, causing failures in SQL queries that require string-to-string comparisons. The fix removes aggressive content-based datetime parsing from the _map_sql_type() method while preserving all existing functionality for actual datetime objects.

Problem

The mssql-python driver was failing on queries like:

cursor.execute("SELECT * FROM table WHERE RIGHT(column, 10) = ?", ('2025-08-12',))

This would throw datetime conversion errors because the driver was automatically trying to parse the string '2025-08-12' as a date object, even when it should remain a string for the RIGHT() function comparison.

Problematic Code (Before Fix):

if isinstance(param, str):
    # Attempt to parse as date, datetime, datetime2, timestamp, smalldatetime or time
    if self._parse_date(param):
        parameters_list[i] = self._parse_date(param)  # MUTATION!
        return (SQL_DATE, SQL_C_TYPE_DATE, 10, 0, False)
    if self._parse_datetime(param):
        parameters_list[i] = self._parse_datetime(param)  # MUTATION!
        return (SQL_TIMESTAMP, SQL_C_TYPE_TIMESTAMP, 26, 6, False)
    # ... more parsing attempts

Why This Was Wrong:

  1. Violated separation of concerns: Type mapping shouldn't do content parsing
  2. Unpredictable behavior: Same string could be treated differently in different contexts
  3. Data mutation: Modified user's parameter data without permission
  4. Against DB-API principles: Parameter binding should be based on Python type, not content

Solution

Approach: Follow PyMSSQL's proven type-based parameter mapping approach.

Key Changes:

  1. Removed automatic datetime parsing from _map_sql_type() for string parameters
  2. Strings now always map to VARCHAR/NVARCHAR based purely on Python type

Files Modified:

  • mssql_python/cursor.py: Simplified string parameter handling in _map_sql_type()
  • tests/test_004_cursor.py: Added test for string parameter behavior

Before vs After

Before (Failed):

date_str = '2025-08-12'
cursor.execute("SELECT * WHERE RIGHT(column, 10) = ?", (date_str,))  # DateTime conversion error

After (Works):

date_str = '2025-08-12'
cursor.execute("SELECT * WHERE RIGHT(column, 10) = ?", (date_str,))  # Works correctly

@gargsaumya gargsaumya changed the title Removed aggressive datetime parsing FIX: Removed aggressive datetime parsing Sep 17, 2025
@gargsaumya
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bewithgaurav bewithgaurav linked an issue Sep 25, 2025 that may be closed by this pull request
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.

This is a significant fix.. Have added one comment related to tests, please make changes and we can merge this off for our next release :)
Thanks for your efforts!

bewithgaurav
bewithgaurav previously approved these changes Sep 26, 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.

This is great! lets fix the conflicts and we should be good to go :)

gargsaumya
gargsaumya previously approved these changes Sep 26, 2025
@arvis108 arvis108 dismissed stale reviews from bewithgaurav and gargsaumya via a95c9f9 September 26, 2025 06:45
@arvis108 arvis108 force-pushed the datetime_conversion_error branch from a95c9f9 to 519fc62 Compare September 26, 2025 06:57
@arvis108
Copy link
Contributor Author

This is great! lets fix the conflicts and we should be good to go :)

Should be fixed, did some messy push, but cleaned it after

@bewithgaurav
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bewithgaurav bewithgaurav merged commit c4b594e into microsoft:main Sep 26, 2025
17 of 18 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.

Datetime conversion error

3 participants