Enable and incorporate metafields by default #132
Conversation
There was a problem hiding this comment.
@fivetran-jamie this PR is looking great. I have a few questions regarding the implementation of the metafields in some downstream use cases that I want to better understand before moving forward.
| shopify_gql_refund_line_item_identifier: "shopify_gql_refund_line_item_data" | ||
| shopify_gql_refund_identifier: "shopify_gql_refund_data" | ||
|
|
||
| # Remove metafields before merging - only relevant to this update |
There was a problem hiding this comment.
Reminder to remove before merge
| -- string agg metafields | ||
| {% if metafields_enabled -%} | ||
| {%- set metafield_columns = adapter.get_columns_in_relation(ref('shopify_gql__customer_metafields')) -%} | ||
| {%- for column in metafield_columns -%} | ||
| {% if column.name.startswith('metafield_') %} | ||
|
|
||
| , {{ fivetran_utils.string_agg(field_to_agg='distinct customers.' ~ column.name, delimiter="', '") }} as {{ column.name }} | ||
|
|
||
| {% endif %} | ||
| {%- endfor %} | ||
| {% endif %} |
There was a problem hiding this comment.
What analytical value are we providing with doing a string agg of the metafields? I would lean towards removing these implementations as I find it hard to derive value in cases like this.
There was a problem hiding this comment.
Let me know your thoughts if there is a case for keeping these aggregations. If not, let's remove the same in the other similar end models.
There was a problem hiding this comment.
If there's a metafield called company and [email protected] is associated with 2 customer_ids, the metafield_company field could look like Fivetran, Waystar Royco. A user could query where metafield_company like '%Fivetran%'. I could see people using this somewhat?
My bigger concern though is that I'm not sure what a good alternative to string-agg'ing would be. We could either use max, a window function to grab the latest metafield value, or not include the metafields here. This is extra unclear to me regarding the collection metafields that currently get string-agg'ed in shopify__products. Taking the max or latest value there doesn't really make sense, as a product can belong to multiple collections
So I lean toward keeping the aggregations, but certainly open to other thoughts
2 participants
PR Overview
Package version introduced in this PR:
This PR addresses the following Issue/Feature(s):
quickstart.ymland join metafields into relevant end models #124Summary of changes:
Submission Checklist
Validation tests have been added and all are passing except for
vertical_integrity_discounts-- discounts were completely untouched by this PR, so this is separate thing that I will look at laterTesting Instructions: Confirm the change addresses the issue(s)
Confirmed that metafield models are running by default and the metafields themselves are being included in relevant end models
Focus Areas: Complex logic or queries that need extra attention
Merge any relevant open PRs into this PR
Changelog