Remove Fabric Dependency #628
Conversation
The 'functional' target in the Makefile now accepts an optional THREADS parameter. If no THREADS value is supplied, it defaults to 'auto'. This allows users to specify the number of parallel test threads, addressing potential resource issues like SQL Server running out of memory when many tests run concurrently.
…rather than taking all the cores.
…sql version of the adapter as the upstream testing was relied on. Fix snapshot logic not being updated to 1.9 to pass tests. Fix issues with the test failure storage tests related to expected errors due to bad model naming.
for more information, see https://pre-commit.ci
|
@Benjamin-Knight I am just an extremely happy user of this adapter but if there is something I could potentially help with please let me know and I will check with my employer if I can carve out some time to work, do tests, etc on this adapter. |
Right now we are kind of on hold until a maintainer appears. Tagging @schlich because he resolved a similar issue in #511. |
|
Yes, I've been swamped with my job and haven't really been able to commit time to this so help would be appreciated. Let me look into if i can add you @Benjamin-Knight as a start |
|
@Benjamin-Knight can you resolve the conflicts on this merge. I merged the other request to get the 3.13 tests to pass but there is some conflicts. |
Done |
…ared to the old locations to be inline with where the fabric adapater stored them to make migrating them all a bit simpler to grok.
|
The previous update by @axellpadilla did some test changes, I also did those but moved the location of the tests so it duplicated a macro, I've fixed that up but there is a new materialization for the unit created in unit_test_table. I do not know the purpose of this but this new materialization is causing some test failures, removing the materialization causes tests to all pass again. |
|
@Benjamin-Knight As I remember, that test materialization was needed because some compatibility issues with upstream, moving out from fabric could mean its not now. |
Ok removed it, all the tests pass my end. |
|
Must admit I'm a bit confused as to why its still trying to do 3.8 unit tests, they are in the workflow any longer. Assume its baked into some sort of pull guard rail. https://github.com/dbt-msft/dbt-sqlserver/blob/master/.github/workflows/unit-tests.yml |
|
You are right, but reviewing it made realize we need to update setup.py |
axellpadilla
left a comment
axellpadilla
left a comment
There was a problem hiding this comment.
Remove python <=3.8 from setup.py
|
Also, for release we should document this change because this will default to dbt docs, and its this way without warning from last version: |
We could also use a behaviour flag to restore the old functionality, if we felt it was that much of a breaking change we could leave the old behaviour in and gate the new behaviour behind a flag to let people opt into it until we deprecate in a later release, this is how DBT core handles this. |
|
I wasn't able to exactly get where this change happened (version), it is indeed breaking (even duplicates dwh data because the different schema name), but I support the idea of behavior change, if we can find the exact part and version that decides if by default or not on this case, on my PRD I needed to leave it by default because moving a whole dwh schema names to new ones is simply not worth |
It was here. Its not listed as a change so I suspect it was not intended. |
4 participants
This pull request is a big one that aims to remove the dependency this adapter has on the dbt-fabric adapter.
As Fabric drifts further from SQL server it will become increasingly difficult to keep Fabric as a base, its also still on version 1.9.
I've also looked at the work @axellpadilla has been doing recently and added in the CI work to support python 3.13 with a view to bumping this adapter up to version 1.11 to match the current DBT core release.
@cody-scott As the most recently active maintainer are you able to help progress this issue or hand additional rights to myself or @axellpadilla to see if we can start closing off some of the issues on this adapter?
In terms of the pull request as its a big one the more eyes on it the better, I've ported over some of the functional tests from Fabric that were not implemented in this adapter to try and keep coverage up and all current functional tests we had are passing. I think we may still want to do a beta release however.