dbt-labs / dbt-adapter-tests

a pytest plugin for dbt adapter test suites
19 stars 11 forks source link

Data test sequence invalid syntax for Oracle DB #17

Closed vitoravancini closed 3 years ago

vitoravancini commented 3 years ago

Hello everyone,

the builtin data test sequence runs this 2 tests:

--passing.sql:
select 1 as id where id = 1

--failing.sql:
select 1 as id where id = 2

This syntax is not valid for Oracle DB, do you have any suggestion on how to overwrite this?

Thank you for this project, made my life a lot easier for testing the adapter. See you!

jtcohen6 commented 3 years ago

Hey @vitoravancini, thanks for your work building an Oracle adapter, and for checking out the test suite!

You can override this right in your spec file, using the projects.overrides key:

projects:
  # data test syntax needs fixing
  - overrides: data_test
    paths:
      test/passing.sql: |
        select 1 as id where true
      test/failing.sql: |
        select 1 as id where false

Out of curiosity, what is the syntactical issue that prevents that code from working on Oracle?

vitoravancini commented 3 years ago

Oracle seems to always need a 'from' statement, had to change another macro somewhere in the adapter implementation because of that.

will try your solution in a bit, thank you!

One more thing, questions about adapter implementation, should I ask in the dbt main project, slack?

the ephemeral materialization for oracle seems very tricky, dbt seems to rely alot in nested ctes for this and Oracle dont allow that.

jtcohen6 commented 3 years ago

One more thing, questions about adapter implementation, should I ask in the dbt main project, slack?

Opening an issue in the main project is better if you have a specific bug report, request, or proposed change to a piece of the core codebase. The #dbt-core-development channel in Slack is the better place for general advice or pointers from other folks who have built adapters, and who may have confronted the same challenges.

the ephemeral materialization for oracle seems very tricky, dbt seems to rely alot in nested ctes for this and Oracle dont allow that.

Yeah, I'm not sure how ephemeral models could/should work on adapters that have limited support for CTEs (or nested CTEs, or CTEs within CTAs, etc). You have a few options, both equally valid:

  1. Declare and document that ephemeral models just aren't supported for your Oracle plugin
  2. Reimplement the compiler and override certain methods around ephemeral CTE interpolation. This looks like overriding get_compiler in dbt/adapters/youradapter/impl.py:
    def get_compiler(self):
        from dbt.adapters.youradapter import YourCompiler
        return YourCompiler(self.config)

    Then you'd create YourCompiler as a subclass of the base dbt Compiler, and reimplement ephemeral-relevant methods like _inject_ctes_into_sql.

This is theoretically possible. I'm just not sure if anyone has managed to do it successfully.

vitoravancini commented 3 years ago

It's a bit selfish of me, but I'll go with the first option. Here at the company I work we have never used ephemeral and looking at the dbt documentation it seems a bit specific the recommended use cases for ephemeral materialization.

It's the only sequence not working from the dbt-adapter-test, I'll release a new version tonight. Is it ok to announce it on slack?

Thank you @jtcohen6 !

jtcohen6 commented 3 years ago

@vitoravancini I think that makes sense! In that case, you can just disable the test_dbt_ephemeral and test_dbt_ephemeral_data_tests steps by commenting them out (or removing them) beneath sequences in your .dbtspec file.

I see you already added to the docs site last fall (https://github.com/fishtown-analytics/docs.getdbt.com/pull/187), nice work following up on that! :)

You're welcome to post about it in the #random channel. We're also in the habit of creating #db-[adapter] channels for folks who want to use a given plugin or follow along; I'd be happy to create a #db-oracle channel, just let me know.

vitoravancini commented 3 years ago

Yeah that would be nice!

I got an email from someone that works at Oracle these days about the adapter and a friend showed me a quora discussion about using dbt with Oracle. The guy asking was trying some incremental model and the adapter didn't support it.

Maybe this release will benefit some people out there.