dbt: Start using dbt-cratedb2 #754
Conversation
amotl
force-pushed
the
dbt-cratedb2
branch
4 times, most recently
from
7d9795b
to
4d6db11
Compare
amotl
force-pushed
the
dbt-cratedb2
branch
5 times, most recently
from
edbbaf5
to
91c0bb9
Compare
amotl
requested review from
hammerhead
and removed request for
kneth and
surister
| {% endmacro %} | ||
|
|
||
| {% macro postgres__create_table_as(temporary, relation, sql) -%} | ||
| {% macro cratedb__create_table_as(temporary, relation, sql) -%} |
There was a problem hiding this comment.
Would not this override be part of dbt-cratedb2?
There was a problem hiding this comment.
In dbt-cratedb2, there is an override already, but using a slightly different strategy, that's why I wanted to discuss it with you if you are good with the new variant.
There was a problem hiding this comment.
If you are good with the new variant?
Those are the corresponding overrides currently shipping with dbt-cratedb2, very closely derived from dbt-postgres, with only necessary amendments applied:
https://github.com/crate/dbt-cratedb2/blob/v0.0.2/dbt/include/cratedb/macros/adapters.sql#L1-L40
In this case, when compared with PostgreSQL, the macro just omits the TEMPORARY modifier in CREATE TEMPORARY TABLE statements.
There was a problem hiding this comment.
the macro just omits the
TEMPORARYmodifier
This is certainly an important detail that should go into the "caveats" section of the documentation.
There was a problem hiding this comment.
With the most recent update, I've removed all macros here.
| @@ -76,7 +47,7 @@ | |||
| ; | |||
| {%- endmacro %} | |||
|
|
|||
| {% macro postgres__rename_relation(from_relation, to_relation) -%} | |||
| {% macro cratedb__rename_relation(from_relation, to_relation) -%} | |||
There was a problem hiding this comment.
Would not this override be something generic we would have as part of dbt-cratedb2?
There was a problem hiding this comment.
Yes, it should definitively go into dbt-cratedb2. Apparently, there is no software test for that in dbt-postgres, otherwise I believe it would have tripped on it?
There was a problem hiding this comment.
I am tracking a second round of improvements over here. That includes harvesting all others of the macros you provided.
There was a problem hiding this comment.
Hi again. I've evaluated the situation around the rename_relation macro on behalf of an integration test case per crate/dbt-cratedb2@3017744, where I responded on inline with the relevant PR, over here.
TLDR; It appears that overriding this macro might no longer be needed, but I also may be wrong.
| {% macro default__reset_csv_table(model, full_refresh, old_relation, agate_table) %} | ||
| {% set sql = "" %} | ||
| {% if full_refresh %} | ||
| {{ adapter.drop_relation(old_relation) }} | ||
| {% set sql = create_csv_table(model, agate_table) %} | ||
| {% else %} | ||
| {{ adapter.truncate_relation(old_relation) }} | ||
| {% set sql = "delete from " ~ old_relation %} | ||
| {% endif %} | ||
|
|
||
| {{ return(sql) }} | ||
| {% endmacro %} | ||
|
|
There was a problem hiding this comment.
Refactored per crate/dbt-cratedb2@3385045. Not in use, yet.
| {% macro generate_schema_name(custom_schema_name, node) -%} | ||
|
|
||
| {%- set default_schema = target.schema -%} | ||
| {%- if custom_schema_name is none -%} | ||
|
|
||
| {{ default_schema }} | ||
|
|
||
| {%- else -%} | ||
|
|
||
| {{ custom_schema_name | trim }} | ||
|
|
||
| {%- endif -%} | ||
|
|
||
| {%- endmacro %} | ||
|
|
There was a problem hiding this comment.
Also refactored, but into the documentation instead.
| {% macro postgres__create_schema(relation) -%} | ||
| {%- call statement('create_schema') -%} | ||
| /* schemas are not created in CrateDB */ | ||
| DROP TABLE IF EXISTS thisschemadefinitelydoesnotexits.thiswouldnotexist | ||
| /* but we need to run something to not have just EOF */ | ||
| {% endcall %} | ||
| {% endmacro %} | ||
|
|
||
| {% macro postgres__create_table_as(temporary, relation, sql) -%} | ||
| {%- set unlogged = config.get('unlogged', default=false) -%} | ||
| {%- set sql_header = config.get('sql_header', none) -%} | ||
|
|
||
| {{ sql_header if sql_header is not none }} | ||
|
|
||
| create table {{ relation }} | ||
| as ( | ||
| {{ sql|replace('"crate".', "") }} | ||
| ); | ||
| {%- endmacro %} | ||
|
|
||
| {% macro postgres__drop_schema(relation) -%} | ||
| {% if relation.database -%} | ||
| {{ adapter.verify_database(relation.database) }} | ||
| {%- endif -%} | ||
| {%- call statement('drop_schema') -%} | ||
| /* schemas are not dropped in CrateDB */ | ||
| {%- endcall -%} | ||
| {% endmacro %} | ||
|
|
||
| {% macro default__drop_relation(relation) -%} | ||
| {% call statement('drop_relation', auto_begin=False) -%} | ||
| drop {{ relation.type }} if exists "{{ relation.schema }}"."{{ relation.identifier }}" | ||
| {%- endcall %} | ||
| {% endmacro %} | ||
|
|
||
| {% macro default__drop_schema(relation) -%} | ||
| {%- call statement('drop_schema') -%} | ||
| /* schemas are not dropped in CrateDB */ | ||
| {% endcall %} | ||
| {% endmacro %} | ||
|
|
||
| {% macro default__create_view_as(relation, sql) -%} | ||
| {%- set sql_header = config.get('sql_header', none) -%} | ||
|
|
||
| {{ sql_header if sql_header is not none }} | ||
| create view "{{ relation.schema }}"."{{ relation.identifier }}" as | ||
| {{ sql|replace('"crate".', "") }} | ||
| ; | ||
| {%- endmacro %} | ||
|
|
There was a problem hiding this comment.
I guess those are being covered well by dbt-cratedb2 now, in turn building upon dbt-postgres. The new locations of corresponding CrateDB overrides is here:
- https://github.com/crate/dbt-cratedb2/blob/v0.0.2/dbt/include/cratedb/macros/adapters.sql
- https://github.com/crate/dbt-cratedb2/blob/v0.0.2/dbt/include/cratedb/macros/relations.sql
Please check if you think something is missing there.
| {% macro postgres__rename_relation(from_relation, to_relation) -%} | ||
| {% do drop_relation(to_relation) %} | ||
| {% set schema_query = "SELECT table_type FROM information_schema.tables WHERE table_schema = '{}' AND table_name = '{}'".format(from_relation.schema, from_relation.identifier) %} | ||
| {% set results = run_query(schema_query) %} | ||
| {% if execute %} | ||
| {% set results_list = results.columns[0].values() %} | ||
| {% else %} | ||
| {% set results_list = [] %} | ||
| {% endif %} | ||
| {% for relation_type in results_list %} | ||
| {% if relation_type == 'VIEW' %} | ||
| {% set view_query = "SELECT view_definition FROM information_schema.views WHERE table_schema = '{}' AND table_name = '{}'".format(from_relation.schema, from_relation.identifier) %} | ||
| {% set view_definitions = run_query(view_query) %} | ||
| {% if execute %} | ||
| {% set view_definitions_list = view_definitions.columns[0].values() %} | ||
| {% else %} | ||
| {% set view_definitions_list = [] %} | ||
| {% endif %} | ||
| {% for view_definition in view_definitions_list %} | ||
| {% call statement('drop_view') -%} | ||
| DROP VIEW IF EXISTS {{ to_relation.schema }}.{{ to_relation.identifier }}; | ||
| {%- endcall %} | ||
| {% call statement('create_view') -%} | ||
| CREATE VIEW {{ to_relation.schema }}.{{ to_relation.identifier }} AS {{ view_definition }} | ||
| {%- endcall %} | ||
| {% call statement('drop_view') -%} | ||
| DROP VIEW IF EXISTS {{ from_relation.schema }}.{{ from_relation.identifier }}; | ||
| {%- endcall %} | ||
| {% endfor %} | ||
| {% else %} | ||
| {% call statement('rename_table') -%} | ||
| ALTER TABLE {{ from_relation.schema }}.{{ from_relation.identifier }} | ||
| RENAME TO {{ to_relation.identifier }} | ||
| {%- endcall %} | ||
| {% endif %} | ||
| {% endfor %} | ||
| {% endmacro %} | ||
|
|
There was a problem hiding this comment.
dbt's rename_relation macro is being explored over at crate/dbt-cratedb2@3017744. Because it seems to work well, as demonstrated by a software test case, it looks like it doesn't need a macro override?
|
Hi. Because most recent amendments to this patch effectively removed all project-specific macro overrides, thus completely relying on dbt-cratedb2, I guess it will be good to merge without much ado. I am keeping the PR open for a bit longer, to give @hlcianfagna the chance to share any objections on relevant inline comments I made above. My intentions are to be very diligent here, in order not to introduce any regressions on dbt details that have been solved already. In general, I think dbt-cratedb2 is in a good condition already. |
About
Validate dbt-cratedb2, the dbt adapter for CrateDB.
Details
This effectively means fragments of the SQL macros provided per Using dbt with CrateDB are being slotted into dbt-cratedb2 now, so they can be removed here.
/cc @surister, @kneth