Set port correctly in mssql connection string #731
Conversation
✅ Deploy Preview for dlt-hub-docs canceled.
|
There was a problem hiding this comment.
fix is good! but arrow test is flaky. IMO
rows = [list(row) for row in select_data(pipeline, f"SELECT * FROM {qual_name} ORDER BY 1")]
you order by _dlt_id which is first AFAIK in the table, then
expected = sorted([list(r.values()) for r in records], key=lambda x: x[0])
you order by first data element (of string type)
and later it works if you are lucky :)
True, I missed that. 😬 |
| result = {k: v for k, v in (param.split('=') for param in dsn.split(";"))} | ||
|
|
||
| assert result == { | ||
| 'DRIVER': 'ODBC Driver 18 for SQL Server', |
There was a problem hiding this comment.
Amazing that the fix came so quickly! :)
I haven't looked into the codebase yet, so maybe my question is misunderstood;
Would this test fail for a developer who only have driver 17 installed?
There was a problem hiding this comment.
Good catch, that's true it would fail.
The test should specify the driver or ignore it in the assert.
|
Is this not included in any release yet? Oh so devel stands for develop, I guess im just confused about what branch pip installs from, probably not the best place for such a question to be placed in comments here :) |
|
@JonasDavisFondene it is not yet included. we still prepare this week's release. we tag the commits with version number so you can see if the PR was merged before or after certain tag. this is not visible in commit history but in the release you can click on the tag and see what got included. not user friendly though... |
|
apart from the Driver 18 test independency suggestion, it would also be nice with some case insensitivity for parameter keys: |
Resolves #721
Fixes odbc connection string to use
SERVER=hostname,portinstead ofSERVER=hostname;PORT=port