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
[Bug] get_relation reporting dbt found an approximate match #5
Comments
Hi @danflippo |
@danflippo I wanted to test
After running
|
For each database I support I have to add a new set of adapter-specific macros. I was testing with an offline branch but I have now published this branch to Github so you can access it. Since it isn't part of a release, you will need to pull in the module directly using the following git syntax: - git: "https://github.com/Snowflake-Labs/dbt_constraints.git"
revision: dbt-oracle-support |
I did some more tracing and I found an additional change. In the Snowflake adapter's impl.py we have the following function: def _make_match_kwargs(self, database, schema, identifier):
quoting = self.config.quoting
if identifier is not None and quoting["identifier"] is False:
identifier = identifier.upper()
if schema is not None and quoting["schema"] is False:
schema = schema.upper()
if database is not None and quoting["database"] is False:
database = database.upper()
return filter_null_values(
{"identifier": identifier, "schema": schema, "database": database}
) This overrides the default implementation in dbt-core: def _make_match_kwargs(self, database: str, schema: str, identifier: str) -> Dict[str, str]:
quoting = self.config.quoting
if identifier is not None and quoting["identifier"] is False:
identifier = identifier.lower()
if schema is not None and quoting["schema"] is False:
schema = schema.lower()
if database is not None and quoting["database"] is False:
database = database.lower()
return filter_null_values(
{
"database": database,
"identifier": identifier,
"schema": schema,
}
) As I mentioned earlier, the databases share the same convention so a non-quoted identifier becomes uppercase and I think this is one of the necessary changes to accommodate this. The dbt-core get_relation() function calls _make_match() and that function calls _make_match_kwargs(). I still believe it is also good to remove your lower() functions in your catalog.sql. I wouldn't be surprised if you had needed those lower() functions in your SQL because you didn't have the logic to override the match function. I think once your override the function you will be able to change your catalog.sql to: select
tables.table_catalog as "table_database",
tables.table_schema as "table_schema",
tables.table_name as "table_name",
tables.table_type as "table_type",
all_tab_comments.comments as "table_comment",
columns.column_name as "column_name",
ordinal_position as "column_index",
case
when data_type like '%CHAR%'
then data_type || '(' || cast(char_length as varchar(10)) || ')'
else data_type
end as "column_type",
all_col_comments.comments as "column_comment",
tables.table_schema as "table_owner"
from tables I would issue a pull-request but I can only imagine the legal red tape I would have to go through to get the Oracle Contribution Agreement signed. Also, these are the steps to recreate my test using dbt-constraint's built-in integration tests:
The way dbt_constraints works, there are normal tests called primary_key, unique_key, and foreign_key that work pretty must as you would expect. These tests do not have any issues in my integration tests. Then after all the models and tests are complete, dbt_constraints has a on-run-end hook that runs the create_constraints macro in create_constraints.sql. This macro loops through all the executed tests and looks for tests that it can turn into an equivalent physical constraint. It is in the beginning of this macro that the macro fails with the approximate match error. I believe the error occurs on line 200:
At this point, the logic has found a test and through its If you search for "model.dbt_constraints_integration_tests.dim_customers", you will find it on line 465. You can find the three attributes I am using to look up the relation on line 490 ("database": "XEPDB1"), 491 ("schema": "DBT_USER") and 501 ("name": "dim_customers"). |
Thank you @danflippo for this detailed analysis. I am going through your recommendations and analysis and will get back to you. |
Hi @danflippo I am testing the dbt_constraints package from the following git branch - git: https://github.com/Snowflake-Labs/dbt_constraints.git
revision: dbt-oracle-support For connection, both user and schema are set to upper case DBT_ORACLE_USER=DBT_TEST
DBT_ORACLE_SCHEMA=DBT_TEST The test cases pass after which dbt_constraints logic is invoked at which point I encounter the following error
My schema.yml looks is as shown below models:
- name: people
columns:
- name: id
tests:
- dbt_constraints.primary_key
- not_null
- unique
- name: gender
tests:
- accepted_values:
values: ['Male', 'Female']
- name: eu_direct_sales_channels_promo_costs
columns:
- name: country_id
tests:
- relationships:
to: ref('countries')
field: country_id
- dbt_constraints.foreign_key:
pk_table_name: ref('countries')
pk_column_name: country_id Please let me know if I am missing something here. |
On a side note, I tested dbt functionalities with both lowercase |
This is the output log of
|
This was reported by another user as well. The issue wasn't related at all to dbt-Oracle. I needed to add one line to verify that a test is not singular test. I have released a new version of dbt Constraints with this fix and I have back-ported the latest changes (including a new test I have for singular tests) into my dbt-oracle-support branch. You should be able to pull in my changes now with |
@danflippo Thank you for updating dbt_constraints. I tested the updates. With
The following table shows all constraints created in the database
However, with
|
- Adapter method _make_match_kwargs() is updated to perform UPPER case comparison - Adapter method quote_seed_column() is updated to not quote seed cols by default - adapter.sql is updated to not lower case metadata - catalog.sql is updated to not lower case metadata - Incremental materialization needed a fix to perform a UPPER case comparison on the unique_key in ON clause - Added a test scenarios in dbt_adbs_test_project to test integration with dbt_constraints - Fixes Bug - #5
You can also install dbt-oracle with latest changes from the git branch https://github.com/oracle/dbt-oracle/tree/fix/integration_with_dbt_constraints We plan to release this in the upcoming dbt-oracle v1.0.3 You can also review the PR #22. |
Hi @danflippo As we are planning to release dbt-oracle's next version late today or early tomorrow, I wanted to check if you had a chance to test the updated dbt-oracle and found any issue integrating with dbt_constraints ? Thanks |
Although this issue was closed automatically after merging the PR, please let me know if you have any questions. |
I'm sorry I didn't get back to you last week. I was on vacation out of the country. The difference above seems to be that it is not finding an exact match for the column in your constraint. I use the following macro to lookup the columns on a relation and compare it to an array of fields for the constraint: {# This macro tests that all the column names passed to the macro can be found on the table, ignoring case #}
{%- macro table_columns_all_exist(table_relation, column_list) -%}
{%- set tab_Columns = adapter.get_columns_in_relation(table_relation) -%}
{%- set tab_column_list = [] -%}
{%- for column in tab_Columns -%}
{{ tab_column_list.append(column.name|upper) }}
{%- endfor -%}
{%- for column in column_list|map('upper') if column not in tab_column_list -%}
{{ return(false) }}
{%- endfor -%}
{{ return(true) }}
{%- endmacro -%} I just published a new version of dbt Constraints and I've back-ported the changes into the oracle support branch. I saw your changes and they all look good to me. I'm going to do some debugging to see if I can identify what was causing your error. |
In the end I found that using the |
Thank you @danflippo for confirming! |
Is there an existing issue for this?
Current Behavior
Hello, I am the author of the dbt_constraints package and I am attempting to add support for dbt-oracle.
I am receiving the following error when my code runs adapter.get_relation():
I have a schema named DBT_USER and I have specified the the schema in upper case in my profile.
I believe the root cause is how dbt-oracle changes the case of objects in your catalog.sql:
I work for Snowflake and our database uses exactly the same conventions as Oracle for how identifiers are treated with and without quotes. Identifiers become upper case if not quoted and case sensitive if quoted. Therefore, I think it can be helpful to compare some of your code against the dbt-Labs code for dbt-Snowflake. In the dbt-Labs implementation of dbt-Snowflake they do not adjust the case of our databases, schemas, tables, or columns in the catalog.sql
There could also be an issue with the oracle__get_columns_in_relation macro:
In the snowflake__get_columns_in_relation macro dbt-labs uses
DESCRIBE TABLE
to retrieve columns and they do not change the case of column names or data types (upper case just like Oracle).I reviewed all the SQL lookups dbt-Labs uses for Snowflake and I consistently see they use UPPER() = UPPER() in their WHERE clauses but in all other cases their metadata lookups preserve the case for databases, schema, tables, and columns. I recommend dbt-Oracle should do the same.
Expected Behavior
No response
Steps To Reproduce
No response
Relevant log output using
--debug
flag enabledNo response
Environment
What Oracle database version are you using dbt with?
Oracle XE 21c
Additional Context
No response
The text was updated successfully, but these errors were encountered: