-
Notifications
You must be signed in to change notification settings - Fork 17
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
Fix renaming the old_relation in Oracle 12c+ when using DBT table materialization with missing database name in profiles.yml #3
Conversation
Thank you @ThoSap for testing and contributing. While we review to get this merged, could you sign the Oracle Contributor Agreement (OCA) ? The bottom of your commit message must have the following line using your name
This can be automatically added to pull requests by committing with:
|
I just wanted to create an Individual OCA but the Can you help @aosingh ? |
Sure, let me check and I will get back to you |
Hi @ThoSap Thanks once again for your help! I would like to better understand the problem and the fixes. The reason is, I am trying to reproduce the issue but it works as expected with the current version of dbt-oracle. Test Model - https://github.com/oracle/dbt-oracle/blob/main/dbt_adbs_test_project/models/promotion_costs.sql I ran the model 3 times and each time it completed successfully. With
|
The thing with this change is that it would no longer be necessary to specify the I don't think the database name property is needed at all as the service name should suffice with these PR changes, maybe we can eliminate it altogether. This probably has historic reasons and also there is the confusion, where if one specifies the profiles.yml property config:
partial_parse: true
printer_width: 120
send_anonymous_usage_stats: false
use_colors: true
dbt_test:
target: prod
outputs:
prod:
type: oracle
protocol: tcp
host: yourhost.example.com
port: 1521
schema: test
user: test
pass: SECRET_PW
# Connection works anyways as in connections.py#L83 the dbname property gets alias database
# and if the service property is not specified the database internally is the service in the end
# https://github.com/oracle/dbt-oracle/blob/v1.0.0/dbt/adapters/oracle/connections.py#L134
dbname: test.service.name.example.com
threads: 4 For my use case using Airbyte (I plan to do there a PR also with the new oracle/dbt-oracle adapter), the |
Also the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we revert the change in dbt/adapters/oracle/impl.py
because in OracleIncludePolicy
the property database
is False. Additionally IncludePolicy
is configurable in dbt_project.yml ?
Thanks for explaining this to me. I understand the following.
Let me know if this understanding is correct. |
Without this change it is not working, even though for |
Ok, I will check this and let you know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to start by keeping dbname
or database
as optional and then eventually remove it. I fear if we immediately get rid of this, it will break things for users who are currently using dbname
or database
.
Maybe I'm missing the technical reason but why was this |
agree and would love to see this simplification/fix getting merged. |
This was present in Techindicium's version and we have kept it for compatibility reasons. I do see your point. Even in ADBS we will not have multiple values for |
Also, even in dbt-core v0.19 (one used by Airbyte), https://github.com/dbt-labs/dbt-core/blob/0.19.latest/core/dbt/contracts/connection.py#L120 |
I will come up with a better solution for these issues. |
Do you have an update on this @aosingh ? |
I'm still trying to fix the following error, help would be appreciated 😁 14:46:57.198298 [error] [MainThread]: Encountered an error:
2022-05-06 14:46:57 normalization > non-default argument 'schema' follows default argument |
…and do not filter on the Oracle DB name for adapter macro oracle__list_relations_without_caching Signed-off-by: Thomas Sapelza <sapelza.thomas@gmail.com> Signed-off-by: Thomas Sapelza <thomas.sapelza@gknpm.com>
…get_columns_in_relation Signed-off-by: Thomas Sapelza <sapelza.thomas@gmail.com> Signed-off-by: Thomas Sapelza <thomas.sapelza@gknpm.com>
…LE_DATABASE as it makes no sense, not even for function adapter.verifiy_database(database) Signed-off-by: Thomas Sapelza <sapelza.thomas@gmail.com> Signed-off-by: Thomas Sapelza <thomas.sapelza@gknpm.com>
Signed-off-by: Thomas Sapelza <sapelza.thomas@gmail.com> Signed-off-by: Thomas Sapelza <thomas.sapelza@gknpm.com>
Signed-off-by: Thomas Sapelza <sapelza.thomas@gmail.com> Signed-off-by: Thomas Sapelza <thomas.sapelza@gknpm.com>
Signed-off-by: Thomas Sapelza <sapelza.thomas@gmail.com> Signed-off-by: Thomas Sapelza <thomas.sapelza@gknpm.com>
…ument Signed-off-by: Thomas Sapelza <sapelza.thomas@gmail.com> Signed-off-by: Thomas Sapelza <thomas.sapelza@gknpm.com>
Signed-off-by: Thomas Sapelza <sapelza.thomas@gmail.com> Signed-off-by: Thomas Sapelza <thomas.sapelza@gknpm.com>
…redentials Signed-off-by: Thomas Sapelza <sapelza.thomas@gmail.com> Signed-off-by: Thomas Sapelza <thomas.sapelza@gknpm.com>
Signed-off-by: Thomas Sapelza <sapelza.thomas@gmail.com> Signed-off-by: Thomas Sapelza <thomas.sapelza@gknpm.com>
Signed-off-by: Thomas Sapelza <sapelza.thomas@gmail.com> Signed-off-by: Thomas Sapelza <thomas.sapelza@gknpm.com>
Signed-off-by: Thomas Sapelza <sapelza.thomas@gmail.com> Signed-off-by: Thomas Sapelza <thomas.sapelza@gknpm.com>
…s set to True Signed-off-by: Thomas Sapelza <sapelza.thomas@gmail.com> Signed-off-by: Thomas Sapelza <thomas.sapelza@gknpm.com>
Signed-off-by: Thomas Sapelza <sapelza.thomas@gmail.com> Signed-off-by: Thomas Sapelza <thomas.sapelza@gknpm.com>
Signed-off-by: Thomas Sapelza <sapelza.thomas@gmail.com> Signed-off-by: Thomas Sapelza <thomas.sapelza@gknpm.com>
de6af15
to
f983c4b
Compare
I signed off all commits using my OCA agreement using rebase, sorry for the commit spam here. |
Superseded by #6 |
Hi,
first of all, thank you very much for releasing an official DBT Oracle DB adapter/profile with an up-to-date DBT version!!! 😍
I tried this adapter today locally together with some Airbyte modifications and found that version 1.0.0 does not rename the already existing destination table to a backup table when using DBT table materialization and therefore I get the following error:
With dbt run --debug
This is caused by the fact that the database name is passed to function
adapter.get_relation()
which should not be the case for Oracle DB.https://github.com/oracle/dbt-oracle/blob/v1.0.0/dbt/include/oracle/macros/materializations/table/table.sql#L22
In Oracle DB the database name is never a prefix when making a select as it does not even work.
✅
schema.relation
❌
database.schema.relation
Also at the DBT/Jinija macro
oracle__list_relations_without_caching
the filter for the table_catalog (database) should be removed as usually no one ever will pass the database name in theprofiles.yml
.Fixes #4
Fixes techindicium/dbt-oracle#8
Fixes techindicium/dbt-oracle#32