FIX: Multi-Statement SQL Enhancement for mssql-python#229
FIX: Multi-Statement SQL Enhancement for mssql-python#229arvis108 wants to merge 5 commits intomicrosoft:mainfrom
Conversation
|
@microsoft-github-policy-service agree |
gargsaumya
changed the title
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
gargsaumya
left a comment
gargsaumya
left a comment
There was a problem hiding this comment.
Hi @arvis108 ,
Thank you for your contribution and for working on this enhancement! We appreciate the effort you’ve put into addressing this.
I noticed that the PR includes the file PR_SUMMARY.md, which isn’t required for this change. While the PR is currently passing all pipeline checks and everything else looks good, it would be great if we could remove this extra file to keep the PR clean and aligned with our project guidelines.
Let me know if you’d like any help or have questions, we’d be happy to assist!
Thanks again for your work on this!
PR_SUMMARY.md
Outdated
| @@ -0,0 +1,299 @@ | |||
| # PR Summary: PyODBC-Style Multi-Statement SQL Enhancement for mssql-python | |||
There was a problem hiding this comment.
Can we please remove this file.
There was a problem hiding this comment.
@arvis108 - thanks a lot for your contribution to the project, this is indeed a detailed implementation of the fix.
I have added some comments requesting a few more testcases and a minor logger change, also I have identified a expected behaviour deviation of pyodbc with this.
Please fel free to add your inputs in the discussion
tests/test_004_cursor.py
Outdated
| drop_table_if_exists(cursor, "dbo.money_test") | ||
| db_connection.commit() | ||
|
|
||
| def test_multi_statement_query(cursor, db_connection): |
There was a problem hiding this comment.
great testcase, can we add a few more tests targetting the below areas
- multiple result sets with multiple select statements on temp tables with
nextset() - semicolons in b/w string literals to ensure no false positives in buffering logic
- multi-statement batch where the final statement is not a SELECT
There was a problem hiding this comment.
Added the tests you mentioned, feel free to add more
tests/test_004_cursor.py
Outdated
| def test_multi_statement_query(cursor, db_connection): | ||
| """Test multi-statement query with temp tables""" | ||
| try: | ||
| # Single SQL with multiple statements - tests pyODBC-style buffering |
There was a problem hiding this comment.
actually - pyodbc strangely gives out below error with this testcase (technically it shouldn't)
pyodbc.ProgrammingError: No results. Previous SQL was not a query.
just curious for discussion, @gargsaumya - if possible, could you please take a look at what pyodbc internally might be doing for this case?
There was a problem hiding this comment.
the simplest repro would be
cursor.execute("""
SELECT 1 as col1, 'test' as col2 INTO #temp1;
SELECT * FROM #temp1;
""")
rows = cursor.fetchall()
print(rows)
pyodbc gives out the error as mentioned in above comment
whereas mssql-python in this PR branch gives out
[(1, 'test')]
There was a problem hiding this comment.
pymssql also gives out [(1, 'test')] with your query
There was a problem hiding this comment.
You are right about pyodbc, I am getting same error now. Looks like when I was testing, I had "SET NOCOUNT ON" set for the session. I am sorry.
There was a problem hiding this comment.
No worries! Since we have a few higher-priority items in progress, we’ll circle back to testing and reviewing this fix a bit later so we can get it merged. Please let us know if this PR is blocking you in any way. Really appreciate your continued efforts to improve the driver.
There was a problem hiding this comment.
PR is not blocking me, no worries
arvis108
force-pushed
the
arvis/multi_statement_feature
branch
from
8ee1a36 to
2fbdab4
Compare
3 participants
Work Item / Issue Reference
Summary
FEAT: Multi-statement SQL support with temp tables
This PR fixes an issue where multi-statement SQL queries (especially those using temporary tables) executed successfully but returned empty result sets in
mssql-python, unlike SSMS and pyodbc.The solution follows pyodbc’s proven approach by automatically applying
SET NOCOUNT ONto multi-statement queries. This preventsDONE_IN_PROCinterference, ensuring correct results without breaking existing functionality.Implementation Highlights
SET NOCOUNT ONinjection incursor.pyBenefits