[Bug] Incremental upsert strategy fails with ORA-00911 & ORA-01747

[ThoSap]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

If a table contains columns names like for example

• "DESC"
• "_AIRBYTE_AB_ID"
• "_AIRBYTE_EMITTED_AT"
• "_AIRBYTE_NORMALIZED_AT"

the dbt-oracle incremental strategy fails with
ORA-01747: invalid user.table.column, table.column, or column specification for example the column name desc -> "DESC"
or
ORA-00911: invalid character which are required internal Airbyte system columns e.g. "_AIRBYTE_AB_ID" or "_AIRBYTE_NORMALIZED_AT".

This is due to the fact that the following macro call returns the columns lowercase and unquoted instead of uppercase and quoted, no matter if the OracleQuotePolicy for schema and identifier is set to true or not.

dbt-oracle/dbt/include/oracle/macros/materializations/incremental/helpers.sql

Lines 37 to 38 in 7348c2b

	{% macro oracle_incremental_upsert(tmp_relation, target_relation, unique_key=none, statement_name="main") %}
	{%- set dest_columns = adapter.get_columns_in_relation(target_relation) -%}

Set schema and identifier to true

dbt-oracle/dbt/include/oracle/dbt_project.yml

Lines 6 to 9 in 7348c2b

	quoting:
	database: false
	schema: false
	identifier: false

dbt-oracle/dbt_adbs_test_project/dbt_project.yml

Lines 8 to 11 in 7348c2b

	quoting:
	database: false
	identifier: false
	schema: false

The two for loops that fill the columns for the two when matched then update set and when not matched then insert(...) values(...) are causing the issue, as they produce for example:

merge into staging.ln_tfgld008 target
  using staging.o$pt_ln_tfgld008125639 temp
  on (temp."_AIRBYTE_AB_ID" = target."_AIRBYTE_AB_ID")
when matched then
  update set
    target.bloc = temp.bloc,
    target.desc = temp.desc, -- desc is not an issue as long it is quoted "DESC"
    target.timestamp = temp.timestamp, -- timestamp suprisingly is not an issue and does not trigger ORA-01747
    target._airbyte_ab_id = temp._airbyte_ab_id, -- triggers ORA-00911 without quoting
    target._airbyte_emitted_at = temp._airbyte_emitted_at, -- triggers ORA-00911 without quoting
    target._airbyte_normalized_at = temp._airbyte_normalized_at, -- triggers ORA-00911 without quoting
    target._airbyte_ln_tfgld008_hashid = temp._airbyte_ln_tfgld008_hashid -- triggers ORA-00911 without quoting
when not matched then
  insert(bloc, desc, timestamp, _airbyte_ab_id, _airbyte_emitted_at, _airbyte_normalized_at, _airbyte_ln_tfgld008_hashid)
  values(
      temp.bloc,
      temp.desc,
      temp.username,
      temp.timestamp,
      temp._airbyte_ab_id,
      temp._airbyte_emitted_at,
      temp._airbyte_normalized_at,
      temp._airbyte_ln_tfgld008_hashid
  )

Expected Behavior

The dbt-oracle incremental upsert strategy should not fail for users that do not have control over the column names of the tables and the column names.
If a column uses a reserved keyword or starts with an invalid character dbt-oracle by default should treat the columns to be quoted and uppercase.

Steps To Reproduce

1. Set the schema and identifier flag to true
   

   dbt-oracle/dbt_adbs_test_project/dbt_project.yml

   Lines 8 to 11 in 7348c2b

   	quoting:
   	database: false
   	identifier: false
   	schema: false

2. Add one of the above columns that trigger the ORA error to https://github.com/oracle/dbt-oracle/blob/7348c2b21883c156583adc8282876c7a993eb13d/dbt_adbs_test_project/seeds/seed.csv
3. Add the column to the incremental materialization test https://github.com/oracle/dbt-oracle/blob/7348c2b21883c156583adc8282876c7a993eb13d/dbt_adbs_test_project/models/us_product_sales_channel_ranking.sql
4. Run the tests https://github.com/oracle/dbt-oracle/blob/7348c2b21883c156583adc8282876c7a993eb13d/dbt_adbs_test_project/README.md

Relevant log output using --debug flag enabled

No response

Environment

- OS: Oracle Linux Server 8.6
- Python: 3.9.13
- dbt: 1.0.6

What Oracle database version are you using dbt with?

Oracle Database 19c Enterprise Edition Release 19.0.0.0.0 - Production
Oracle Database 19c Standard Edition 2 Release 19.0.0.0.0 - Production

Additional Context

No response

[ThoSap]

This also relates in a way to #5

[ThoSap]

In my opinion, dbt-oracle should treat the schema and identifier to be quoted and uppercase at all times.
Imagine if you have a user that only has read access to a table and therefore is not able to properly fix
ORA-01747: invalid user.table.column, table.column, or column specification
or
ORA-00911: invalid character

Or for the use case of Airbyte where from a completely differnet RDBMS (in my case a read-only data lake), where the column name desc is allowed, is transformed into a JSON and then normalized into a table in Oracle DB.
This is also the case for the use case of Airbyte's system internal column names that trigger ORA-00911, which could be fixed by quoting and uppercase of the column names.

Products like jOOQ do this and I have not had an issue with this for years.

[aosingh]

Thank you for the detailed bug report @ThoSap

The correct way to fix this should be that, dbt should quote identifiers as per quoting configuration defined in dbt_project.yml. if the dbt project does not specify quoting configuration then we should fall back to a reasonable default that Oracle recommends and database users understand.

Let me check this and I will keep you posted

In my opinion, dbt-oracle should treat the schema and relation to be quoted and uppercase at all times.

[sfc-gh-dflippo]

I agree that the project should respect the quote parameters. But a side effect of respecting them is that if you turn on quoting right now, a lot of stuff starts breaking because of the LOWER() that was added to the metadata lookup SQL. I tried to work around my issue with dbt_constraints by turning quoting on and off and nothing helped. Honestly, if you were going to alter the case, you would do better to make everything UPPER() since that is what Oracle will do if you do not quote the identifiers. The best solution is to allow quoting on and off and to leave the case alone in all the metadata lookup queries.

[ThoSap]

@aosingh do you have an update on this?

[aosingh]

Hi @ThoSap

I have been constantly thinking about this one. I would like to better support quoting in dbt-oracle

Currently, if you read the documentation on Database Object Names and Qualifiers, it says

Note:

Oracle does not recommend using quoted identifiers for database object names. 

I also discussed this specific issue with dbt-core team in the slack channel

That being said, the current changes in the PR #9 assumes that columns will always be quoted and upper case. This makes the identifiers case sensitive. This will fail with ORA-00904: invalid identifier for models which do not want case sensitivity.

{%- if unique_key is not none -%}
    merge into {{ '"{}"'.format(target_relation).replace('.', '"."') | upper }} target
      using {{ '"{}"'.format(tmp_relation).replace('.', '"."') | upper }} temp
      on (temp.{{ unique_key }} = target.{{ unique_key }})

[ThoSap]

@aosingh I agree, better quoting support is the way to go 👍🏼
Regarding quoting not being recommended, as I previously stated there are cases where a user unfortunately has no choice but to use the columns with reserved keywords or columns starting with special characters.

Should I change my PR so we at least only have the Jinja whitespace control improvements (remove explicit quoting and uppercase)?

[aosingh]

Yeah sure. I would recommend closing the PR #9 and creating a fresh PR with the name Jinja whitespace control improvements against the base branch release/v1.0.3. We are working on release v1.0.3 and will freeze it soon.