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

[ELE-51] Microsoft SQL Server (mssql, dbt-sqlserver) integration #353

Open
agn-sta opened this issue Oct 6, 2022 · 18 comments
Open

[ELE-51] Microsoft SQL Server (mssql, dbt-sqlserver) integration #353

agn-sta opened this issue Oct 6, 2022 · 18 comments

Comments

@agn-sta
Copy link

agn-sta commented Oct 6, 2022

Like the above: Are you planning to extend elementary for dbt-sqlserver adapter?

ELE-51

@Maayan-s
Copy link
Contributor

Maayan-s commented Oct 6, 2022

Hi @agn-sta,
Thanks for opening the issue!
We currently don't officially support dbt-sqlserver, but Elementary dbt package is built on top of dbt so it shouldn't be crazy hard to integrate with it.
That said, we prioritize integrations based on demand from the community, so we will track this issue and see if there is more demand.

You are of course welcome to try and implement it yourself, I'll be happy to provide guidance!

@agn-sta
Copy link
Author

agn-sta commented Oct 7, 2022

Okay, thank you for a quick answer ;)

@Maayan-s Maayan-s changed the title Question: Are you planning to extend elementary for dbt-sqlserver adapter? Microsoft SQL Server (mssql, dbt-sqlserver) integration Oct 19, 2022
@rtooker
Copy link

rtooker commented Oct 26, 2022

@Maayan-s what would be involved in this? Is there another port I can look at to see the scale of what would be required to pick this up?

@Maayan-s
Copy link
Contributor

Hi @rtooker,
First of all, thank you for wanting to contribute to the project!
I would be happy to provide guidance.
To be honest it's hard for me to asses, as dbt-sqlserver is not an official adapter.
Some of the community adapters have all of the functionality the official ones have, but some don't.
If we depend on a functionality that is missing from this adapter, it means an integration would require to add it (to elementary or to the adapter).

Generally speaking, we implemented every platform-specific functionality using adapter.dispatch, as dbt recommends. You can see an example in this macro.
Anywhere there was an adapter native function or dbt_utils macro that we could use, we did.

This might cause a problem with SQL Server as dbt_utils does not support it. Since dbt 1.2.0 many of the macros were migrated from dbt_utils to the adapter code, but I'm not sure if these are all implemented in the dbt-sqlserver adapter.
According to the latest release notes some of these were added, so I'm optimistic.
Anyway, You can see here a workaround we did for such a gap with Databricks.

We recently decided (due to demand from the community) to add a Databricks integration, and approached it gradually -

Step 1 - Add support for uploading dbt artifacts and run results (in the dbt package).
Step 2 - Add support in the CLI for Slack alerts and UI generation.
Step 3 - Add support for data anomaly detection test (the most complex and platform-specific part of the code right now).

You can check out this PR where I implemented step 1 for Databricks. As you can see it actually required pretty minor changes.
If you want to give a shot with SQL Server, I would be happy to support you!

@rtooker
Copy link

rtooker commented Nov 2, 2022

Thanks @Maayan-s . I'll have a look at step 1 on Friday. I'm hoping it's straightforward. tsql-utils shims some of the functionality of dbt_utils and as you say the latest versions of dbt-sqlserver seem to have accounted for the 1.2.0 changes 🤞

@rtooker
Copy link

rtooker commented Nov 4, 2022

At first glance there are a lot of errors when running the e2e tests against sql server but many of them are similar.

One of the most common errors seems to be related to sql server not having a boolean data type.

A couple of questions:

  1. I can add a macro here for sql server to use bit (the closest thing to boolean), but is it ok to add code to your repo for a target that's not officially supported?
  2. Adding bit only gets me so far - I still need to translate True to 1 and False to 0 in the generated sql (e.g. generated from here) if the target is sqlserver. Any hints on where to start on that?

@elongl
Copy link
Member

elongl commented Nov 9, 2022

Hi @rtooker.
Thanks a lot for working on this.

Regarding your questions -

  1. Yes, it's okay! For starters, we'll add the MSSQL adapter as community-based support, so you're right in saying that it will not yet be officially supported.
  2. We can change queries that use true and false to use 1=1 or 1=0.

What do you think?

@G14rb
Copy link

G14rb commented Nov 9, 2022

Error also for the sample query from limit to top and the datediff (not sure if from 1.3 dbt.utils solved it)... Also timestamp type gave me headache

@elongl
Copy link
Member

elongl commented Nov 9, 2022

@rtooker @G14rb
We can also introduce a more gradual support by not including all of Elementary's features on the initial implementation. For instance, we can exclude the anomaly detection tests and only support the dbt artifacts which should be much easier. Of course this depends on which features of Elementary you're interested in.

Here's an example of how we can exclude parts of the E2E tests and potentially address them later if we'd like.

@G14rb
Copy link

G14rb commented Nov 9, 2022

Nice, are you going to add the adapter for both packages (elementary and dbt-data-reliability), right?

@Maayan-s
Copy link
Contributor

Yes :)

@haritamar
Copy link
Collaborator

Hi @rtooker ,

Hope you are well!
Just checking in if you are still working on this and if we can help in any way.
Thanks!

@rtooker
Copy link

rtooker commented Nov 23, 2022

Hi @haritamar ,

I had to leave it for a few weeks but I got back to it today and have fixed maybe 10 compatibility issues - https://github.com/elementary-data/dbt-data-reliability/compare/master...rtooker:dbt-data-reliability:sql-server-initial-support?expand=1

Still quite a few issues though, just a painful process of unpicking them one by one. Hopefully I'll get a few more hours on it on Friday.

Robert

@rtooker
Copy link

rtooker commented Nov 23, 2022

There's probably a decision to be made on which repo some of these fixes should end up in, but I want to get a clean run of the integration tests first.

@tford06
Copy link

tford06 commented Dec 15, 2022

Has there been an update on this? Thanks!

@Maayan-s
Copy link
Contributor

Hi @tford06 ,
I see that this fork is still in progress:
https://github.com/marvinfromblueforte/dbt-data-reliability

@marvinfromblueforte isn't on the thread but maybe @rtooker can share if they are working on it together?

@marvinfromblueforte
Copy link

marvinfromblueforte commented Dec 20, 2022

A couple days ago I had a look at @rtooker 's fork to see if I can help too. If I remember correctly, the biggest remaining issue is MSSQL's lack of support for nested CTEs and GROUP BY numbers. I think the former could be a showstopper because according to this open issue it requires reworking the SQL compiler in the dbt-sqlserver package.

However, I still think that limited functionality (excluding anomaly testing) should be possible. I'd like to give this another try after the holidays.

@rtooker Do you currently have any plans to keep working on this issue?

(Edit: mistakenly mentioned tford06 instead of rtooker)

@Hadarsagiv Hadarsagiv changed the title Microsoft SQL Server (mssql, dbt-sqlserver) integration [ELE-51] Microsoft SQL Server (mssql, dbt-sqlserver) integration Jan 3, 2023
@Hadarsagiv Hadarsagiv added the Contribution Created by Linear-GitHub Sync label Jan 3, 2023
@unstoppable-allensun
Copy link

Anyone still working on this? I would love to use Elementary for just test result collection but we are currently using SQL Server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants