search

techindicium / dbt-oracle

A dbt adapter for oracle db backend
Other
38 stars 18 forks source link

Migrate to dbt v1.0.3 #30

Closed patped closed 2 years ago

patped commented 2 years ago

Migrate adapter to work with dbt v1.0.3. The work is mostly based on @eredzik fork of the adapter and he should get all the kudos for this work. My only "contribution" is to try getting the changes pulled upstream.

patped commented 2 years ago

There are still some work to be done before the PR is ready.

Tests are still failing because of the implementation of error_if and warn_if in v0.20.0. @eredzik: I noticed that you have managed to fix this issue. Do you want to make an isolated commit with a fix or point me to what changes that needs to be done? 😄

I also need to go over the changelog to see of there are other changes and new features that needs adjusting before the work is ready.

@vitoravancini: Is there anything else you want me to do or want me / us to do differently to make this PR pass?

eredzik commented 2 years ago

Thanks @patped for moving this issue forward! The issue of error_if and warn_if if i remember correctly was fixed by changes in macros/tests/helpers.sql get_test_sql and test_not_null macro. I would also suggest using change in rename_relation macro as oracle doesn't allow for direct renaming of views - you have to get definition of the view and recreate it (rename_relation macro).

patped commented 2 years ago

Thanks for the info! 💯 I can give you write access to https://github.com/patped/dbt-oracle if you want to do it yourself? 😃

patped commented 2 years ago

I pushed a work in progress commit where I added the macros/tests/helpers.sql file and it's still not working. 🤔Can you do a review of my code to see what I missed?

patped commented 2 years ago

I would also suggest using change in rename_relation macro as oracle doesn't allow for direct renaming of views - you have to get definition of the view and recreate it (rename_relation macro).

It works with no issues on our database and the docker image gvenzl/oracle-xe:18. What version are you running on? Anyway, I think this PR should manly focus on migrating the adapter and issues with renaming should separated. Make a new issue and we can discuss it further there.

patped commented 2 years ago

I pushed a work in progress commit where I added the macros/tests/helpers.sql file and it's still not working. 🤔Can you do a review of my code to see what I missed?

I've forgot to include the file in the package.

patped commented 2 years ago

I've tried to go through the changelogs from v0.20.x up to v.1.0.x and the only thing I want to check more thoroughly is get_where_subquery introduced in v0.21.1.

mf36 commented 2 years ago

Hi guys, I very much appreciate your efforts - kudos :). Do you already know if with the migration changes the snapshot feature will also work properly? It seems to be buggy and not working with the initial adapter. Cheers!

patped commented 2 years ago

@mf36: I haven't tested or tried to fix the snapshot feature. I have only focused on migrating the existing version of the adapter to 1.0.x and not fixing bugs that's already there.

That said I'm interested in hearing about these kinda bugs so make an issue so we can discuss it further. 😃

patped commented 2 years ago

Oracle has now made offisial support for dbt and I see no point to merge this PR

Ref: https://github.com/oracle/dbt-oracle