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

Feature/sqlserver support #14

Closed
wants to merge 3 commits into from

Conversation

b-per
Copy link
Contributor

@b-per b-per commented Mar 16, 2021

Hi.

I have added support to SQL Server and modified one existing macro (date() doesn't exist in SQL Server).
Integration tests passed OK.

In order to run, the project also needs the package dbt-msft/tsql_utils

Cheers,

@clausherther
Copy link
Contributor

Hi @b-per, this is great, thanks for that PR! Since there are quite a few macros to add, plus a new package dependency, I wonder if it would make more sense to turn this into its own package, similar to tsql-utils and spark-utils? I was planning on doing the same for the few Spark-specific macros I have in here and move to them to something like dbt-date-spark. So, the main dbt-date package would continue to only target the 3 core platforms we currently support (Snowflake, BigQuery, Redshift/Postgres).

@b-per
Copy link
Contributor Author

b-per commented Mar 16, 2021

Hi! It makes complete sense.

With dbt-utils backed up with tsql-utils and spark-utils, what do you think about dbt-date expanded with tsql-date and spark-date. We lose the context that it is about dbt but we follow the same convention as existing packages.

I'll have a look at how tsql-utils and spark-utils have been created and give a go with the code I have in the PR.

@clausherther
Copy link
Contributor

You're right, those packages follow a different naming convention, and I would argue they should have been named dbt-utils-spark etc to make for more clear namespacing. But if you're creating the package, naming is of course up to! ;)

@b-per
Copy link
Contributor Author

b-per commented Mar 17, 2021

Hi @clausherther!

I got all the smaller macros to work in my new adapter repo but need some work on the more complex ones that are used in dim_date.sql and dim_date_fiscal.sql. I will make the repo public then.

For now, all integration tests are passing but the models dim_date.sql and dim_date_fiscal.sql are raising SQL errors, due to the fact that the macros leverage CTEs a lot and nested with are not supported in SQL Server.

I will close this PR and may raise a smaller one for changes that would affect all SQL engines.

@b-per b-per closed this Mar 17, 2021
@clausherther
Copy link
Contributor

Cool, very excited to see your new package!

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

Successfully merging this pull request may close these issues.

2 participants