Incorporate 1.7.2 changes #461

cody-scott · 2023-12-19T18:17:48Z

Updates adapter to 1.7.2

for more information, see https://pre-commit.ci

cody-scott · 2023-12-19T20:57:07Z

One piece here about two tests related to the full refresh of seeds.

It doesn’t seem to want to drop the table to pass that materializations test.

Not sure if tests want to be revisited to see if there is any tests that are covered in the parent adapter. Kept the existing tests here and they passed (calling the fabric macros where not otherwise defined).

ericmuijsvanoord · 2024-01-02T07:11:10Z

@cody-scott :
Thanks for the hard work!
What is pending the merge of the code? Reviews? Fixing the full refresh of seeds? Additional tests?

cody-scott · 2024-01-02T20:33:34Z

I believe we are waiting on the repo maintainers to take a look.

Further work is needed to add the full suite of tests that DBT provides, and to see if that full-refresh failure can be corrected (or ignored) for now.

ericmuijsvanoord · 2024-01-04T07:18:47Z

@cody-scott : can you tag the maintainers as reviewers or is that not possible ?

schlich · 2024-01-08T03:57:53Z

Hey @cody-scott !!! Can't believe i missed this PR. I believe I have maintainer status on the repo -- I volunteered a couple months ago to help this along. really happy to see an attempt here -- i've been hacking away at my own fixes but it seems like we can start from yours!!

Leaving comments in a "review" note below.

~~also, my dev container crashed on startup so i might be suggesting some changes there.~~ nvm, didnt realize you were using docker-in-docker so it was just codespaces not working

Thanks for the help!

schlich

This looks good -- My only question is about the commented out tests; as long as we are passing the existing tests i'm not too worried about inspecting the application code; we can address those issues as they come up and we gotta prioritize getting this out. As soon as I get a response about the missing tests I can get CI running!

tests/unit/adapters/sqlserver/test_sql_server_connection_manager.py

schlich · 2024-01-08T04:45:48Z

one more thing... on my machine i'm getting failures for the 3 test_docs tests in addition to the two i believe you mentined for test_seed... dont have time to dig in tonight but i'm attaching test report logs:

test_results.json

JCZuurmond · 2024-01-08T11:11:12Z

@sdebruyn and @dataders : Could you have a look? We are waiting for this update

schlich · 2024-01-08T17:26:14Z

@sdebruyn and @dataders : Could you have a look? We are waiting for this update

@JCZuurmond sdebruyn is no longer maintaining this repo and I believe dataders is on parental leave until march... I believe I am your point of contact for now :)

cody-scott · 2024-01-16T02:18:12Z

Ok, i'm seeing 5 failed @schlich from your json file.

TestBasicSeedTestsSQLServer.test_simple_seed_full_refresh_flag
TestBasicSeedTestsSQLServer.test_simple_seed_full_refresh_flag
TestDocsGenReferencesSQLServer.test_references
TestDocsGenerateSQLServer.test_run_and_generate
TestDocsGenerateSQLServer.test_run_and_generate_no_compile

In terms of some of the failing azure tests, I think most of that should pull from fabric, but not positive here (might need to look closer). We aren't using azure so i don't have a reliable way to test on my side.

Is there an appetite to add some of the other tests that dbt has introduced since 1.4? I believe there is a number more in their suite now.

Next steps i'm seeing:

Sort out why the full refresh seeds aren't working
See if there is a link to the docs and full-refresh issue; if not look at that
Sort out the azure auth component for those using service principals/entra

cody-scott · 2024-01-20T19:37:18Z

Two questions here:

The behaviour of the default seed full refresh test seems to be expecting the ~~table~~ view to not exist. This is not the case as its dropping + creating right away in the log file for that step. The default implementation then won't pass.
In the fabric adapter they have over-riden most (or all, i haven't checked the full set) of the tests to have it expecting the table to exist https://github.com/microsoft/dbt-fabric/blob/main/tests/functional/adapter/test_seed.py#L599

The fabric adapter seems to have re-written

many of the tests and not implemented the default test suite from dbt; same problem they're having?

I am puzzled here; it seems like we should be expecting the table to exist (like what the fabric one does)
The default implementation doesn't seem to have changed, so unless something has changed on the seed/full refresh side then the way forward would be updating those tests that are failing to pass, as the full refresh is recreating the tables.

@schlich what are you thinking here?

second step of test dbt.log

Log file for only the second step (run_dbt seed full refresh) where its pretty clear its dropping + creating.

Edit:

See my latest comment.
Default behaviour is drop cascade in postgres + others. Fabric doesn't drop cascade, only drop, hence the fail.

cody-scott · 2024-01-20T19:42:53Z

One approach here would be to re-align with the same test set of fabric or try and get the tests in line with the current dbt suite + any one-off modifications as required.

cody-scott · 2024-01-20T20:25:25Z

Ok, i think i've got the issue; the downstream view isn't being dropped as part of the full refresh. Not sure exactly what changed but that seems to be the issue here. Maybe its something fabric is doing that isn't aligning...

cody-scott · 2024-01-21T15:52:38Z

Alright I think this is the state.

The base tests are expecting that it performs like postgres and should perform a 'drop view/table cascade', which would explain why the test is expected to have no view on a full refresh.

Since sql server doesn't support this I see two options

Implement a similar cascade drop by determining the downstream views with this sys.sql_expression_dependencies. This would be the closest representation to the expected DBT behaviour. 2. Ignore this and pass the tests with it expecting the tables, don't drop the views until that step is run in the build.

Upside of the first is its closest to the expected DBT, downside is it's going to drop all downstream tables and views. If you have anything that relies on them in build then it's going to remove them until it hits that step. relevant discussion.

Other piece is non dbt controlled views will also get dropped.

Fabric is going for option 2.

@schlich What's your thoughts? It's tied to full refresh so that's probably why those particular items are failing (I think).

schlich · 2024-01-22T19:50:48Z

Amazing legwork! Based on that bug report you linked, which seems like its a completely valid concern, I'd say option 2 is the way to go. Let me know if you need help with the code; I'm totally swamped of late so i appreciate you taking this up.

Where are we at on the CLI login you think?

@cody-scott

prescode · 2024-01-22T21:37:24Z

I also would like to volunteer to help if I can. Give me a task and I'll make a run at it. Thanks so much for your work on this @cody-scott !

prescode · 2024-01-25T03:12:47Z

Ok, i'm seeing 5 failed @schlich from your json file.

TestBasicSeedTestsSQLServer.test_simple_seed_full_refresh_flag

TestBasicSeedTestsSQLServer.test_simple_seed_full_refresh_flag

TestDocsGenReferencesSQLServer.test_references

TestDocsGenerateSQLServer.test_run_and_generate

TestDocsGenerateSQLServer.test_run_and_generate_no_compile

In terms of some of the failing azure tests, I think most of that should pull from fabric, but not positive here (might need to look closer). We aren't using azure so i don't have a reliable way to test on my side.

Is there an appetite to add some of the other tests that dbt has introduced since 1.4? I believe there is a number more in their suite now.

Next steps i'm seeing:

Sort out why the full refresh seeds aren't working

See if there is a link to the docs and full-refresh issue; if not look at that

Sort out the azure auth component for those using service principals/entra

The doc errors

TestDocsGenerateSQLServer.test_run_and_generate

TestDocsGenerateSQLServer.test_run_and_generate_no_compile

are related to the time_type data type in the test fixture:

    @pytest.fixture(scope="class")
    def expected_catalog(self, project):
        return base_expected_catalog(
            project,
            role=os.getenv("DBT_TEST_USER_1"),
            id_type="int",
            text_type="varchar",
            time_type="datetime",
            view_type="VIEW",
            table_type="BASE TABLE",
            model_stats=no_stats(),
        )

The fixture in dbt-fabric uses datetime2.
Updating to datetime2 results in a pass

    @pytest.fixture(scope="class")
    def expected_catalog(self, project):
        return base_expected_catalog(
            project,
            role=os.getenv("DBT_TEST_USER_1"),
            id_type="int",
            text_type="varchar",
            time_type="datetime2",
            view_type="VIEW",
            table_type="BASE TABLE",
            model_stats=no_stats(),
        )

I'm fairly certain this is the correct fix but being unfamiliar with the code I'm looking for confirmation. Any insight into this update?

prescode · 2024-01-25T04:24:31Z

I had hoped to get further along but I'm struggling to figure out this testing scheme and trace the tests. I've overridden some of the test fixtures but I'm still getting failures. I'll not have much time in the next couple of days to continue but I'll pick it up again on the weekend if needed.

schlich · 2024-01-25T04:40:55Z

The doc errors

TestDocsGenerateSQLServer.test_run_and_generate

TestDocsGenerateSQLServer.test_run_and_generate_no_compile

are related to the time_type data type in the test fixture:

    @pytest.fixture(scope="class")
    def expected_catalog(self, project):
        return base_expected_catalog(
            project,
            role=os.getenv("DBT_TEST_USER_1"),
            id_type="int",
            text_type="varchar",
            time_type="datetime",
            view_type="VIEW",
            table_type="BASE TABLE",
            model_stats=no_stats(),
        )

The fixture in dbt-fabric uses datetime2. Updating to datetime2 results in a pass

    @pytest.fixture(scope="class")
    def expected_catalog(self, project):
        return base_expected_catalog(
            project,
            role=os.getenv("DBT_TEST_USER_1"),
            id_type="int",
            text_type="varchar",
            time_type="datetime2",
            view_type="VIEW",
            table_type="BASE TABLE",
            model_stats=no_stats(),
        )

I'm fairly certain this is the correct fix but being unfamiliar with the code I'm looking for confirmation. Any insight into this update?

I've run into the same error in other contexts, I can confirm this fix.

Can you push up a PR of your WIP against this branch so i can examine and potentially merge?

for more information, see https://pre-commit.ci

…o dev

prescode · 2024-01-25T15:36:35Z

@schlich it looks like you beat me to it. We don't need to merge my changes anymore, correct? I also made a change to the devcontainer.json file to update the path to the test.env file based on our above conversation. Let me know if there is anything else I can do to help.

schlich · 2024-01-25T17:17:17Z

@mikaelene SOS if you are around. We need an organization owner to remove the dependency on Python 3.7 in the required checks. Will see if anyone has a hotline to @dataders otherwise, or we will have to wait until he returns from parental leave and explore an unofficial pypi release in the meantime. Unless anyone else is seeing anything i'm not!

But we're almost there!!!!

b-per · 2024-01-26T11:40:08Z

Hi @schlich , I might be able to help while Anders is out.
Can you tell me where you see the dependency on Python 3.7 in the required checks?

schlich · 2024-01-26T11:51:08Z

Hi @b-per it was a busy day yesterday! I was able to obtain admin perms to the repo to remove the 3.7 check myself. Today we are stuck with the pyPI release. Do you have access there?

b-per · 2024-01-26T14:00:44Z

No, I don't have access to this one.
Looking at the git blame the release action was originally set by @sdebruyn and there a few maintainers on PyPi but I am not in the list

sdebruyn · 2024-01-26T14:09:56Z

I updated the token in the GH secrets, I don't have access to add other folks myself.

cody-scott and others added 10 commits December 17, 2023 10:03

update versions to 1.7

7be1b00

migrated work for fabric connection

f3ded8b

added devcontainer and updated development files

bb8009e

update connection managers

fb41851

remove macros applied in fabric

19480fb

update sql server specific macros

e719d71

updated tests

e4a7869

updated changes and contrib

5394dc7

added clone sql

7c2c1cd

[pre-commit.ci] auto fixes from pre-commit.com hooks

9e84072

for more information, see https://pre-commit.ci

cody-scott mentioned this pull request Dec 19, 2023

Update on the support and future of this adapter #441

Open

cody-scott mentioned this pull request Dec 20, 2023

Fabric version of dbt-sqlserver #456

Closed

schlich reviewed Jan 8, 2024

View reviewed changes

tests/unit/adapters/sqlserver/test_sql_server_connection_manager.py Outdated Show resolved Hide resolved

mhurenkamp-groveko mentioned this pull request Jan 9, 2024

Can't use dbt-sqlserver as part of poetry managed project due to incompatible version declaration #462

Open

ericmuijsvanoord mentioned this pull request Jan 16, 2024

Fix concat macro when fields list is of length 1. #447

Open

all tests passing locally

b36e242

schlich mentioned this pull request Jan 25, 2024

Dev #464

Closed

Ty Schlichenmeyer and others added 12 commits January 25, 2024 06:09

fix: relax python version for pre commit

6af1ef5

[pre-commit.ci] auto fixes from pre-commit.com hooks

b69565b

for more information, see https://pre-commit.ci

drop python 3.7 support

7483ff1

Merge branch 'dev' of https://github.com/dbt-msft/dbt-sqlserver into dev

d0a5623

drop azure CI action

09a83a5

drop support for python 3.7 in integration tests

bd50fb7

dont cascade drop downstream views

fb32960

[pre-commit.ci] auto fixes from pre-commit.com hooks

3d3050e

for more information, see https://pre-commit.ci

[pre-commit.ci] auto fixes from pre-commit.com hooks

7dfd63b

for more information, see https://pre-commit.ci

overwrite for cascade exception

7e900be

Merge branch 'dev' of https://github.com/dbt-msft/dbt-sqlserver into dev

9818d69

Merge branch 'dev' of https://github.com/cody-scott/dbt-sqlserver int…

dae9e93

…o dev

Ty Schlichenmeyer added 4 commits January 25, 2024 10:55

try 3.7 unit tests again

dc22f68

downgrade pre-commit

2af0ce2

revert 3.7 check

28e0cfb

restore pre-commit version

66fedda

schlich enabled auto-merge January 25, 2024 17:45

schlich merged commit 2421fe9 into dbt-msft:master Jan 25, 2024
53 checks passed

cody-scott deleted the dev branch February 11, 2024 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incorporate 1.7.2 changes #461

Incorporate 1.7.2 changes #461

cody-scott commented Dec 19, 2023

cody-scott commented Dec 19, 2023

ericmuijsvanoord commented Jan 2, 2024

cody-scott commented Jan 2, 2024

ericmuijsvanoord commented Jan 4, 2024

schlich commented Jan 8, 2024 •

edited

Loading

schlich left a comment

schlich commented Jan 8, 2024

JCZuurmond commented Jan 8, 2024

schlich commented Jan 8, 2024 •

edited

Loading

cody-scott commented Jan 16, 2024

cody-scott commented Jan 20, 2024 •

edited

Loading

cody-scott commented Jan 20, 2024

cody-scott commented Jan 20, 2024

cody-scott commented Jan 21, 2024

schlich commented Jan 22, 2024 •

edited

Loading

prescode commented Jan 22, 2024

prescode commented Jan 25, 2024

prescode commented Jan 25, 2024

schlich commented Jan 25, 2024 •

edited

Loading

prescode commented Jan 25, 2024

schlich commented Jan 25, 2024

b-per commented Jan 26, 2024

schlich commented Jan 26, 2024

b-per commented Jan 26, 2024

sdebruyn commented Jan 26, 2024

Incorporate 1.7.2 changes #461

Incorporate 1.7.2 changes #461

Conversation

cody-scott commented Dec 19, 2023

cody-scott commented Dec 19, 2023

ericmuijsvanoord commented Jan 2, 2024

cody-scott commented Jan 2, 2024

ericmuijsvanoord commented Jan 4, 2024

schlich commented Jan 8, 2024 • edited Loading

schlich left a comment

Choose a reason for hiding this comment

schlich commented Jan 8, 2024

JCZuurmond commented Jan 8, 2024

schlich commented Jan 8, 2024 • edited Loading

cody-scott commented Jan 16, 2024

cody-scott commented Jan 20, 2024 • edited Loading

cody-scott commented Jan 20, 2024

cody-scott commented Jan 20, 2024

cody-scott commented Jan 21, 2024

schlich commented Jan 22, 2024 • edited Loading

prescode commented Jan 22, 2024

prescode commented Jan 25, 2024

prescode commented Jan 25, 2024

schlich commented Jan 25, 2024 • edited Loading

prescode commented Jan 25, 2024

schlich commented Jan 25, 2024

b-per commented Jan 26, 2024

schlich commented Jan 26, 2024

b-per commented Jan 26, 2024

sdebruyn commented Jan 26, 2024

schlich commented Jan 8, 2024 •

edited

Loading

schlich commented Jan 8, 2024 •

edited

Loading

cody-scott commented Jan 20, 2024 •

edited

Loading

schlich commented Jan 22, 2024 •

edited

Loading

schlich commented Jan 25, 2024 •

edited

Loading