fivetran / dbt_hubspot_source

Data models for Hubspot built using dbt.
https://fivetran.github.io/dbt_hubspot_source/
Apache License 2.0
31 stars 30 forks source link

[Bug] Use preserved word as alias in `stg_hubspot__contact` #124

Open papost opened 7 months ago

papost commented 7 months ago

Is there an existing issue for this?

Describe the issue

In the stg_hubspot__contact there is the following macro, whose purpose is to rename the columns that have as prefix property and remove it. However, for the column property_delete the result of this transformation is delete. However, this world is preserved leading to unexpected behavior. {{ fivetran_utils.remove_prefix_from_columns( columns=adapter.get_columns_in_relation(ref('stg_hubspot__contact_tmp')), prefix='property_', exclude=get_macro_columns(get_contact_columns())) }} {% endif %}

Relevant error log or model output

Database Error in model stg_hubspot__contact (models/stg_hubspot__contact.sql)
  001003 (42000): SQL compilation error:
  syntax error line 652 at position 27 unexpected 'DELETE'.
  compiled Code at target/run/hubspot_source/models/stg_hubspot__contact.sql

Expected behavior

The expected behavior is to give a different naming convention to this column. For example, is_contact_deleted

dbt Project configurations

dbt version: 1.7.4 adapter: snowflake 1.7.1

Package versions

Package version 0.14.0

What database are you using dbt with?

snowflake

dbt Version

dbt version: 1.7.4

Additional Context

No response

Are you willing to open a PR to help address this issue?

fivetran-jamie commented 7 months ago

Hi there! Thanks for the taking the time to file this issue, it definitely makes sense. Couple of questions -- I presume that you have the hubspot__pass_through_all_columns variable set to true?

Also, you mentioned that you'd expect this column to be aliased as is_contact_deleted. I believe there is already a field aliased as such (_fivetran_deleted). Does this value always map onto the value of property_delete? Mostly asking just for my reference

fivetran-jamie commented 7 months ago

i was able to recreate the issue and resolve it by adjusting our macro to wrap the outputted delete field with quotes -- would that be an acceptable solution for you instead of aliasing?

also are you using just the hubspot_source package or also the hubspot transform package? i have a testing branch for either -- if you agree with the above approach and would be interesting in testing it out

fivetran-jamie commented 7 months ago

here are the branches btw

# in packages.yml
packages:
## if only using hubpot_source
- git: https://github.com/fivetran/dbt_hubspot_source.git
  revision: explore/remove-prefix-no-reserved-words
  warn-unpinned: false 
## if using transform (do not include above)
- git: https://github.com/fivetran/dbt_hubspot.git
  revision: explore/remove-prefix-no-reserved-words
  warn-unpinned: false 
papost commented 7 months ago

Hi, and thank you for the response! You correctly presumed that I have the hubspot__pass_through_all_columns variable set to true. You are right about the _fivetran_deleted column, it is aliased as is_contact_deleted, so the delete column is different. Regarding your proposal I don't think it is the best solution, as I said above delete is a preserved word in SQL and under no circumstances should be used, either in quotes or not.

also are you using just the hubspot_source package or also the hubspot transform package? i have a testing branch for either -- if you agree with the above approach and would be interesting in testing it out

I use both of them.

fivetran-jamie commented 7 months ago

Ah yeah you have a very good point.

Question - do you care about having the property_ prefix removed? We originally included it by default because, a while back, Hubspot suddenly added the prefix to people's properties and the change broke their downstream reports

fivetran-jamie commented 7 months ago

just bumping this 😄 @papost

fivetran-jamie commented 6 months ago

marking this as stale for now, but happy to continue thinking this through if folks are interested!