Skip to content
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

Update adaptors.sql to use tblproperties macro #848

Merged

Conversation

etheleon
Copy link
Contributor

@etheleon etheleon commented Aug 3, 2023

resolves #865
resolves #190
docs dbt-labs/docs.getdbt.com/#

Problem

tblproperties clause macro is not used when creating tables. Add macro to adaptor.sql

Solution

Add macro

Checklist

  • [ x] I have read the contributing guide and understand what's expected of me
  • [x ] I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

@etheleon etheleon requested a review from a team as a code owner August 3, 2023 01:32
@cla-bot cla-bot bot added the cla:yes label Aug 3, 2023
@etheleon
Copy link
Contributor Author

etheleon commented Aug 3, 2023

see discussion #847

typo in name of macro
Copy link
Collaborator

@JCZuurmond JCZuurmond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etheleon : Thanks for the contribution! I added a suggestion to align the code with the other clauses:

@@ -69,6 +69,7 @@
{{ clustered_cols(label="clustered by") }}
{{ location_clause() }}
{{ comment_clause() }}
{{ dbt_spark_tblproperties_clause() }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etheleon : Could you rename the dbt_spark_tblproperties_clause to spark__tblproperties_clause and create a similar pattern as for the other clauses?

{% macro tblproperties_clause() %}
  {{ return(adapter.dispatch('tblproperties_clause', 'dbt')()) }}
{%- endmacro -%}

{% macro spark__tblproperties_clause() %}
...
{%- endmacro -%}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I've submitted my commit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JCZuurmond if you have time for this review

* changed macro `dbt_spark_tblproperties_clause()` to `spark__tblproperties_clause()`
* create new macro `tblproperties_clause()` which references the above
@etheleon etheleon force-pushed the incl_tblproperties_clause_in_seed branch from f017a0b to 3bd8916 Compare August 3, 2023 17:32
Copy link
Collaborator

@JCZuurmond JCZuurmond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for fixing this

@JCZuurmond
Copy link
Collaborator

@Fleid: Could you merge this PR?

@@ -69,6 +69,7 @@
{{ clustered_cols(label="clustered by") }}
{{ location_clause() }}
{{ comment_clause() }}
{{ tblproperties_clause() }}
Copy link

@martelli martelli Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same problem and fixed it in a similar way locally. My question is then: why add it here (seed.sql) and not in adapters.sql directly under macro spark__create_table_as?
It seems it would be a more generic change, no?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, @martelli. It makes more sense to put it in the spark__create_table_as. I assumed it was there, did not read correctly that the change was done in the `seed.sql.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JCZuurmond Ok. Since I had the fix ready, I've completed a parallel pull request. If this one gets updated and landed, I'll discard mine.

@etheleon
Copy link
Contributor Author

etheleon commented Aug 9, 2023

@colin-rogers-dbt can you review this? 🙇

@etheleon etheleon changed the title Update seed.sql include tblproperties Update adaptors.sql to use tblproperties macro Aug 9, 2023
@Fleid
Copy link
Contributor

Fleid commented Aug 9, 2023

@JCZuurmond are we doing this one?
Looks like the changelog entry is missing.

Copy link
Contributor

@colin-rogers-dbt colin-rogers-dbt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we tested this manually? I don't think our existing tests cover this case. At the very least we should open an issue to update our automated tests.

@colin-rogers-dbt
Copy link
Contributor

@etheleon can you add a changelog entry?

@etheleon
Copy link
Contributor Author

etheleon commented Aug 9, 2023

hey @colin-rogers-dbt working on it 👍

  • added changelog
  • yes tested locally with jaffle shop example. ran SHOW CREATE TABLE to get the table definition now it includes tblproperties
/*
    Welcome to your first dbt model!
    Did you know that you can also configure models directly within SQL files?
    This will override configurations stated in dbt_project.yml

    Try changing "table" to "view" below
*/

{{
    config(
        location_root='s3a://some_bucket/tables',
        tblproperties = {
          'format': 'iceberg/parquet',
          'format-version': '2',
          'write.delete.mode': 'merge-on-read',
          'write.delete.target-file-size-bytes': '536870912',
          'write.merge.mode': 'merge-on-read',
          'write.metadata.delete-after-commit.enabled': 'true',
          'write.metadata.previous-versions-max': '100',
          'write.target-file-size-bytes': '536870912',
          'write.update.mode': 'merge-on-read'}
    )
}}

with source_data as (

    select 1 as id
    union all
    select null as id

)

select *
from source_data

/*
    Uncomment the line below to remove records with null `id` values
*/

-- where id is not null

output from SHOW CREATE TABLE

CREATE TABLE Iceberg catalog.ds.my_first_dbt_model ( id INT) USING iceberg LOCATION 's3a://some_bucket/tables/my_first_dbt_model' TBLPROPERTIES ( 'current-snapshot-id' = '1344159441507971776', 'format' = 'iceberg/parquet', 'format-version' = '2', 'write.delete.mode' = 'merge-on-read', 'write.delete.target-file-size-bytes' = '536870912', 'write.merge.mode' = 'merge-on-read', 'write.metadata.delete-after-commit.enabled' = 'true', 'write.metadata.previous-versions-max' = '100', 'write.target-file-size-bytes' = '536870912', 'write.update.mode' = 'merge-on-read')

@colin-rogers-dbt colin-rogers-dbt enabled auto-merge (squash) August 9, 2023 18:45
@colin-rogers-dbt colin-rogers-dbt merged commit 6000540 into dbt-labs:main Aug 9, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants