Skip to content
This repository has been archived by the owner on May 4, 2022. It is now read-only.

Fix list relations macro #14

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

LewisDavies
Copy link

First of all, thanks for building this adapter - appreciate the hard work! I've made some changes that should make incremental loading work properly. The main problem was that adapter.get_relation wasn't returning any results, which I traced back to the oracle__list_relations_without_caching macro.

The database and schema arguments were being changed to uppercase, but the output of sys.all_tables and sys.all_views was not. I've updated all comparisons in adapters.sql to use uppercase and the macros now provide usable output.

I haven't been able to run all tests yet because I'm having environment issues, but wanted to open a PR for feedback anyway.

@vitoravancini
Copy link
Collaborator

@LewisDavies very sorry for taking this long to reply, thank you for the pull request!

I'll have a look this weekend.

I'm not sure if making everything upper case might cause some issue when using quoting for the relations names.

Are you aware of the quoting configs @LewisDavies ?
@dpavancini any thoughts on this?

@LewisDavies
Copy link
Author

Hey @vitoravancini, no worries. I'm aware the quoting options in the dbt profile but I'm not sure how to incorporate them here. Happy to help test this if you've got any suggestions.

@vitoravancini
Copy link
Collaborator

I've started bringing the newer dbt integrationt tests for adapter last night(https://github.com/fishtown-analytics/dbt-adapter-tests)
but I did not see anything about quoting.

My suggestion for testing quoting options with incremental mode for now is using the dbt_test_project included in this repo. Maybe create a model with a mix of uppercase and lowercase column names, and try to make some assertion using dbt test on this columns.

Does that makes sense?

@jeffw82
Copy link

jeffw82 commented Feb 12, 2021

@vitoravancini does this mean you are also working on making the dbt-oracle compatible with newer versions of dbt? Would like to use the dbt run state filters in v18+, but dbt-oracle is currently supporting up to dbt 17.2. Either way, thanks much for your efforts on this project.

@vitoravancini
Copy link
Collaborator

@jeffw82 yes we intend to make this oracle adapter compatible with 18+ dbt versions

@LewisDavies
Copy link
Author

I just tried using the test project to test my changes but haven't been able to do so; the project seems tailored to a specific environment.

It would take a non-trivial amount of work to getting it running with my setup and meaningfully test an incremental table. I'm leaving my current role next week so I can't dedicate the time to it, and I won't have access to an Oracle DB afterwards.

Anything I can do to help before then?

@vitoravancini
Copy link
Collaborator

@LewisDavies can you tell more about your setup? And when you say the test project are you talking about the test project in this repo or the dbt integration test?

I was able to use the dbt integration tests(https://github.com/fishtown-analytics/dbt-adapter-tests) and runs most tests sucessfully. I'll merge it tonight.

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

Successfully merging this pull request may close these issues.

3 participants