Datavault-UK / automate-dv

A free to use dbt package for creating and loading Data Vault 2.0 compliant Data Warehouses (powered by dbt, an open source data engineering tool, registered trademark of dbt Labs)
https://www.automate-dv.com
Apache License 2.0
511 stars 131 forks source link

[BUG] cast failure in replace_placeholder_with_period_filter on `to_timestamp()` call #239

Closed igorvoltaic closed 3 months ago

igorvoltaic commented 3 months ago

Describe the bug cast fails for anything other than varchar in replace_placeholder_with_period_filter on to_timestamp() call https://github.com/Datavault-UK/automate-dv/blob/d56ce03a655efd2db87e405e3a5f3f1b365a2ac0/macros/materialisations/period_mat_helpers/replace_placeholder_with_period_filter.sql#L23-L25

Environment

dbt version: 1.8 automate_dv version: latest Database/Platform: Trino

Expected behavior cast should work as expected

AB#5589

azure-boards[bot] commented 3 months ago

✅ Successfully linked to Azure Boards work item(s):

DVAlexHiggs commented 3 months ago

Thanks for this report. At this time, I'm closing this issue due to two reasons:

If you wish to continue using the custom materialisation, please see here for how to add support for the correct cast approach yourself, however I strongly recommend against using these materialisations.

Please re-open this if necessary in the future. Thank you!

igorvoltaic commented 3 months ago

Well, that was quick! Thanks for the response. I still believe correct cast should be used piece instead of hardcoding that: TO_TIMESTAMP({{ timestamp_field }}) -> cast({{ timestamp_field }} as {{ dbt.type_timestamp() }})

DVAlexHiggs commented 3 months ago

Well, that was quick! Thanks for the response. I still believe correct cast should be used piece instead of hardcoding that: TO_TIMESTAMP({{ timestamp_field }}) -> cast({{ timestamp_field }} as {{ dbt.type_timestamp() }})

In AutomateDV, the default is used by Snowflake and we have platform specific implementations as needed, so the code you linked is correct for the Snowflake version, hence it's hard-coded. We were planning to change the implementation (if it had not been deprecated) to use your suggested approach of delegating the casting to a cross-platform macro.

The current approach of using Snowflake as the default can be confusing. We are going to be changing this in future versions, where the default will raise an unimplemented error/warning instead.