[CT-2492] [Bug] Update snapshot logic to use dbt.hash instead of hardcoded md5

[graciegoheen]

Is this a new bug in dbt-core?

• I believe this is a new bug in dbt-core
• I have searched the existing issues, and I could not find an existing issue for this bug

Current Behavior

Currently, dbt_scd_id is built with a hardcoded md5().

Expected Behavior

Instead, dbt_scd_id should be built using {{ dbt.hash(...) }} to remove hard-coded md5().

Steps To Reproduce

Run a dbt snapshot

Relevant log output

No response

Environment

- OS:
- Python:
- dbt:

Which database adapter are you using with dbt?

No response

Additional Context

No response

[jtcohen6]

@graciegoheen Are you thinking about other data platforms that don't support md5, or support it slightly differently?

IIRC, BigQuery is one such example. Looks like we've reimplemented this macro over there:

{% macro bigquery__snapshot_hash_arguments(args) -%}
  to_hex(md5(concat({%- for arg in args -%}
    coalesce(cast({{ arg }} as string), ''){% if not loop.last %}, '|',{% endif -%}
  {%- endfor -%}
  )))
{%- endmacro %}

When we could just be using the cross-db hash() utility, and saving ourselves the trouble!

[graciegoheen]

That's right. For your BigQuery example, we already have a hash override for BigQuery, so also having the snapshot_hash_arguments override feels redundant:

{% macro bigquery__hash(field) -%}
    to_hex({{dbt.default__hash(field)}})
{%- endmacro %}

The other different here for BigQuery's snapshot_hash_arguments is that it requires the elements to be cast to string (instead of varchar in the default), but couldn't that instead be handled by using dbt.type_string?

It looks like Spark does something similar for snapshot_hash_arguments.

[dbeatty10]

Consider me a fan of cross-database implementations! 💪

If a more cross-database implementation of default__snapshot_hash_arguments is added to dbt-core, it would probably involve dbt.type_string, dbt.concat, and dbt.hash as mentioned by @graciegoheen.

The implementation might feel a bit complicated due to dbt.concat taking a Jinja array of strings as its argument. I'm guessing that list would need to be built up prior to passing it into dbt.concat, which might make the logic harder to read and understand.

But would be nice if this worked out so we could remove more hard-coded SQL function names from dbt-core.

[github-actions]

This issue has been marked as Stale because it has been open for 180 days with no activity. If you would like the issue to remain open, please comment on the issue or else it will be closed in 7 days.

[github-actions]

Although we are closing this issue as stale, it's not gone forever. Issues can be reopened if there is renewed community interest. Just add a comment to notify the maintainers.