Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql server ADO driver bugs on windows #10637

Merged
merged 2 commits into from
Nov 19, 2021
Merged

Conversation

djova
Copy link
Contributor

@djova djova commented Nov 15, 2021

What does this PR do?

Fixes several bugs with the SQL Server integration when running on Windows with the MSOLEDBSQL driver:

  • cursor.execute was not handling a list of parameters with ADO as varargs like it does with ODBC. Update to pass the arguments so it works in all cases.
  • The python2 ADO provider was failing to properly pass on raw bytes to the PLAN_LOOKUP_QUERY so the query is now updated to receive the hashes as hex strings and have the database do the conversion to bytes.

Additional testing improvements:

  • add MSOLEDBSQL to the list of valid ADO providers to test
  • install the MSOLEDBSQL driver in the azure test VM
  • update the SQL Server test VM setup to match what is used for the SQL Server linux docker tests
  • run the test_statement_metrics_and_plans tests for both the SQL Server Windows local VM as well as the Docker VM

Motivation

Fixes bugs with the SQL Server integration when running on Windows with the MSOLEDBSQL driver.

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #10637 (bee0621) into master (cd2c541) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Flag Coverage Δ
sqlserver 84.92% <100.00%> (+0.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

djova added a commit that referenced this pull request Nov 18, 2021
If a file in the pipeline setup scripts directory starts with an underscore, then it is treated as a "non-executable" file and it will not be executed.

This is required for #10637 where a large SQL setup script is needed to be referenced by one of the other scripts, but the SQL script itself must not be executed.
djova added a commit that referenced this pull request Nov 18, 2021
Required for #10637

The test suite improvements in that PR which expand Windows test coverage are what surfaced this bug.
djova added a commit that referenced this pull request Nov 19, 2021
If a file in the pipeline setup scripts directory starts with an underscore, then it is treated as a "non-executable" file and it will not be executed.

This is required for #10637 where a large SQL setup script is needed to be referenced by one of the other scripts, but the SQL script itself must not be executed.
@djova djova force-pushed the djova/sqlserver-fix-bug branch 2 times, most recently from 59edc7d to 0f6a047 Compare November 19, 2021 01:33
djova added a commit that referenced this pull request Nov 19, 2021
* fix `mmh3.hash64` unicode exception with python2 on Windows

Required for #10637

The test suite improvements in that PR which expand Windows test coverage are what surfaced this bug.

* add unicode test case
Fixes several bugs with the SQL Server integration when running on Windows with the `MSOLEDBSQL` driver:
*  `cursor.execute` was not handling a list of parameters with ADO as varargs like it does with ODBC. Update to pass the arguments so it works in all cases.
* The python2 ADO provider was failing to properly pass on raw bytes to the `PLAN_LOOKUP_QUERY` so the query is now updated to receive the hashes as hex strings and have the database do the conversion to bytes.

Additional testing improvements:
* add `MSOLEDBSQL` to the list of valid ADO providers to test
* install the ``MSOLEDBSQL driver in the azure test VM
* update the SQL Server test VM setup to match what is used for the SQL Server linux docker tests
* run the `test_statement_metrics_and_plans` tests for both the SQL Server Windows local VM as well as the Docker VM
@djova djova force-pushed the djova/sqlserver-fix-bug branch from 0f6a047 to b8c637a Compare November 19, 2021 01:51
@djova djova marked this pull request as ready for review November 19, 2021 01:54
@djova djova requested a review from a team as a code owner November 19, 2021 01:54
@djova djova changed the title sql server fix bug sql server ADO bugs on windows Nov 19, 2021
@djova djova changed the title sql server ADO bugs on windows sql server ADO driver bugs on windows Nov 19, 2021
@fanny-jiang fanny-jiang added the category/bugfix For use during Agent Release period label Nov 19, 2021
@fanny-jiang fanny-jiang merged commit 4dd0c5e into master Nov 19, 2021
@fanny-jiang fanny-jiang deleted the djova/sqlserver-fix-bug branch November 19, 2021 15:23
fanny-jiang pushed a commit that referenced this pull request Nov 19, 2021
* fix `mmh3.hash64` unicode exception with python2 on Windows

Required for #10637

The test suite improvements in that PR which expand Windows test coverage are what surfaced this bug.

* add unicode test case

(cherry picked from commit cd2c541)
fanny-jiang pushed a commit that referenced this pull request Nov 19, 2021
* sql server fix windows ado bugs

Fixes several bugs with the SQL Server integration when running on Windows with the `MSOLEDBSQL` driver:
*  `cursor.execute` was not handling a list of parameters with ADO as varargs like it does with ODBC. Update to pass the arguments so it works in all cases.
* The python2 ADO provider was failing to properly pass on raw bytes to the `PLAN_LOOKUP_QUERY` so the query is now updated to receive the hashes as hex strings and have the database do the conversion to bytes.

Additional testing improvements:
* add `MSOLEDBSQL` to the list of valid ADO providers to test
* install the ``MSOLEDBSQL driver in the azure test VM
* update the SQL Server test VM setup to match what is used for the SQL Server linux docker tests
* run the `test_statement_metrics_and_plans` tests for both the SQL Server Windows local VM as well as the Docker VM

* bump checks base minimum version

(cherry picked from commit 4dd0c5e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants