Bugfix/redshift constant expressions #92

fivetran-reneeli · 2024-11-20T20:41:23Z

PR Overview

This PR will address the following Issue/Feature: v0.14.0

This PR will result in the following new package version:

Breaking, potential changes to the schema

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

Under the Hood

Adds enable config for the upstream metadata staging model (stg_shopify__metafield). For more information on how to enable/disable this table, refer to the README.
Adds disable config for the upstream abandoned_checkout staging models (including stg_shopify__abandoned_checkout, stg_shopify__abandoned_checkout_discount_code, and stg_shopify__abandoned_checkout_shipping_line). For more information on how to enable/disable these tables, refer to the README.

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

dbt run –full-refresh && dbt test
dbt run (if incremental models are present) && dbt test

Before marking this PR as "ready for review" the following have been applied:

The appropriate issue has been linked, tagged, and properly assigned
All necessary documentation and version upgrades have been applied
docs were regenerated (unless this PR does not include any code or yml updates)
BuildKite integration tests are passing
Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

Recreate the constant expressions error in integration_test folder by running on an empty schema ( ex: shopify_schema: empty). If the upstream table is empty, it should be disabled and voided in the downstream compiled code

If you had to summarize this PR in an emoji, which would it be?

💃

fivetran-joemarkiewicz

@fivetran-reneeli thanks for working through these changes. Please see my change requests and notes below. A number of them reference the source PR so I would recommend reviewing that PR first. Let me know if you have any questions. Thanks!

fivetran-joemarkiewicz · 2024-11-22T19:59:11Z

CHANGELOG.md

+# dbt_shopify v0.14.0
+
+## Under the Hood
+- Adds enable config for the upstream `metadata` staging model (`stg_shopify__metafield`). For more information on how to enable/disable this table, refer to the [README](https://github.com/fivetran/dbt_shopify/blob/main/README.md#adding-metafields).


See my notes from the source, we will need to rethink the approach taken for the metafields.

I updated the approach and documentation, let me know your thoughts!

fivetran-joemarkiewicz · 2024-11-22T19:59:27Z

CHANGELOG.md

+
+## Under the Hood
+- Adds enable config for the upstream `metadata` staging model (`stg_shopify__metafield`). For more information on how to enable/disable this table, refer to the [README](https://github.com/fivetran/dbt_shopify/blob/main/README.md#adding-metafields).
+- Adds disable config for the upstream `abandoned_checkout` staging models (including `stg_shopify__abandoned_checkout`, `stg_shopify__abandoned_checkout_discount_code`, and `stg_shopify__abandoned_checkout_shipping_line`). For more information on how to enable/disable these tables, refer to the [README](https://github.com/fivetran/dbt_shopify/blob/main/README.md#step-5-disable-models-for-non-existent-sources).


See my source PR review note and make the same updates here.

fivetran-joemarkiewicz · 2024-11-22T20:05:59Z

README.md

@@ -120,7 +120,18 @@ vars:
    shopify_using_fulfillment_event: true # false by default
 ```

-### Step 5: Setting your timezone
+### Step 5: Disable models for non-existent sources


See my note from the source package about consolidating this with the above section

consolidated

fivetran-joemarkiewicz · 2024-11-22T20:10:53Z

README.md

@@ -172,7 +183,7 @@ vars:
 ```

 #### Adding Metafields
-In [May 2021](https://fivetran.com/docs/applications/shopify/changelog#may2021) the Shopify connector included support for the [metafield resource](https://shopify.dev/api/admin-rest/2023-01/resources/metafield). If you would like to take advantage of these metafields, this package offers corresponding mapping models which append these metafields to the respective source object for the following tables: collection, customer, order, product_image, product, product_variant, shop. If enabled, these models will materialize as `shopify__[object]_metafields` for each respective supported object. To enable these metafield mapping models, you may use the following configurations within your `dbt_project.yml`.
+In [May 2021](https://fivetran.com/docs/applications/shopify/changelog#may2021) the Shopify connector included support for the [metafield resource](https://shopify.dev/api/admin-rest/2023-01/resources/metafield). If you would like to take advantage of these metafields, this package offers corresponding mapping models which append these metafields to the respective source object for the following tables: collection, customer, order, product_image, product, product_variant, shop. Enabling any of the following variables will materialize the `stg_shopify__metafield` model, in addition to respective models that will materialize as `shopify__[object]_metafields` for each respective supported object. To enable these metafield mapping models, you may use the following configurations within your `dbt_project.yml`.


See my previous notes about how we should rethink this approach

fivetran-joemarkiewicz · 2024-11-22T20:11:26Z

integration_tests/dbt_project.yml

+  shopify_using_fulfillment_event: true
+  shopify_using_all_metafields: true
+  shopify__standardized_billing_model_enabled: true
+  shopify_using_abandoned_checkout: true


Is this necessary? Isn't it enabled by default already?

Ah yes it is, removed

fivetran-joemarkiewicz · 2024-11-22T20:11:38Z

integration_tests/dbt_project.yml

+  shopify_using_fulfillment_event: true
+  shopify_using_all_metafields: true
+  shopify__standardized_billing_model_enabled: true


We need to comment these out

commented out

fivetran-joemarkiewicz · 2024-11-22T20:12:13Z

integration_tests/dbt_project.yml

@@ -56,7 +57,7 @@ dispatch:
    search_order: ['spark_utils', 'dbt_utils']

 models:
-  +schema: "{{ 'shopify_integrations_tests_sqlw' if target.name == 'databricks-sql' else 'shopify' }}"
+  +schema: "{{ 'shopify_integrations_tests_sqlw' if target.name == 'databricks-sql' else 'shopify_customers_dev' }}"


Can we change this back?

oops- switched

fivetran-joemarkiewicz · 2024-11-22T20:13:59Z

packages.yml

+#   version: [">=0.13.0", "<0.14.0"]
+
+  - git: https://github.com/fivetran/dbt_shopify_source.git
+    revision: bugfix/redshift_constant_expressions
+    warn-unpinned: false


Reminder to swap before release

fivetran-joemarkiewicz

@fivetran-reneeli great work on this. I have just a few more requests before approval.

fivetran-joemarkiewicz · 2024-11-26T16:01:53Z

CHANGELOG.md

+   - `stg_shopify__abandoned_checkout_discount_code`
+   - `stg_shopify__abandoned_checkout_shipping_line`. 
+- For more information on how to enable/disable these tables, refer to the [README](https://github.com/fivetran/dbt_shopify/blob/main/README.md#step-4-disable-models-for-non-existent-sources).
+- Updates the `index` calculation in `stg_shopify__abandoned_checkout_discount_code` by removing the conditional logic for null scenarios now that a disable config has been added to the model.


This is not relevant for the transform model. We can remove this entry.

Suggested change

- Updates the `index` calculation in `stg_shopify__abandoned_checkout_discount_code` by removing the conditional logic for null scenarios now that a disable config has been added to the model.

Right! Removed

fivetran-joemarkiewicz · 2024-11-26T16:07:53Z

CHANGELOG.md

+- Adds enable/disable config for the `metadata` staging model using the `shopify_using_metafield` variable (default `true`).
+- Adds enable/disable config for the `abandoned_checkout` staging models using the `shopify_using_abandoned_checkout` variable (default `true`):
+   - `stg_shopify__abandoned_checkout`
+   - `stg_shopify__abandoned_checkout_discount_code`
+   - `stg_shopify__abandoned_checkout_shipping_line`. 


We should also callout the downstream impacts of these variables within this package. For example, the metafield variable is now a requirement in the metafield tables and the abandoned checkout variable will turn off relevant information in X end models.

good point,updated

README.md

.quickstart/quickstart.yml

fivetran-joemarkiewicz

LGTM with just one small whitespace suggestion in the CHANGELOG

CHANGELOG.md

Co-authored-by: Joe Markiewicz <[email protected]>

fivetran-catfritz

@fivetran-reneeli a small suggestion but lgtm!

fivetran-catfritz · 2024-11-26T22:34:46Z

CHANGELOG.md

+# dbt_shopify v0.14.0
+
+[PR #92](https://github.com/fivetran/dbt_shopify/pull/92) includes the following updates:
+## Under the Hood


Suggested change

## Under the Hood

## Breaking Changes

Same here since this could result in schema changes.

fivetran-reneeli added 4 commits November 19, 2024 20:27

add abandoned_checkout config, update docs and versions

72ad389

eventually change the deps

ba126b0

update docs, schema, script

05c7aef

updates

15848f4

fivetran-reneeli self-assigned this Nov 20, 2024

fix seed data identifier

1edf140

fivetran-joemarkiewicz self-requested a review November 22, 2024 19:57

fivetran-joemarkiewicz requested changes Nov 22, 2024

View reviewed changes

fivetran-reneeli added 6 commits November 25, 2024 16:35

add metadata config and update readme

c940dfa

update changelog

9a3b810

docs and revert project configs

7ebcdb5

changelog

abb3770

update run script

1863934

new schema

b301152

fivetran-reneeli requested a review from fivetran-joemarkiewicz November 25, 2024 23:41

small update to readme align

851ef8c

fivetran-joemarkiewicz requested changes Nov 26, 2024

View reviewed changes

fivetran-reneeli added 2 commits November 26, 2024 12:40

update readme

1f4755f

updates

699bcb8

fivetran-reneeli requested a review from fivetran-joemarkiewicz November 26, 2024 19:11

fivetran-joemarkiewicz approved these changes Nov 26, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

fivetran-reneeli and others added 2 commits November 26, 2024 14:09

Update CHANGELOG.md

933eb4b

Co-authored-by: Joe Markiewicz <[email protected]>

format updates

fc86cef

fivetran-reneeli linked an issue Nov 26, 2024 that may be closed by this pull request

[Documentation] Move badges below H1 heading #91

Closed

4 tasks

fivetran-catfritz approved these changes Nov 26, 2024

View reviewed changes

fivetran-reneeli and others added 2 commits November 26, 2024 17:31

update

bb82d51

package ref

89efa4c

fivetran-jamie merged commit 295a919 into main Dec 2, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/redshift constant expressions #92

Bugfix/redshift constant expressions #92

fivetran-reneeli commented Nov 20, 2024 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Nov 22, 2024

fivetran-reneeli Nov 25, 2024

fivetran-joemarkiewicz Nov 22, 2024

fivetran-reneeli Nov 25, 2024

fivetran-reneeli Nov 25, 2024

fivetran-joemarkiewicz Nov 22, 2024

fivetran-reneeli Nov 25, 2024

fivetran-reneeli Nov 25, 2024

fivetran-joemarkiewicz Nov 22, 2024

fivetran-reneeli Nov 25, 2024

fivetran-joemarkiewicz Nov 22, 2024

fivetran-reneeli Nov 25, 2024

fivetran-joemarkiewicz Nov 22, 2024

fivetran-reneeli Nov 25, 2024

fivetran-joemarkiewicz Nov 22, 2024

fivetran-reneeli Nov 25, 2024

fivetran-reneeli Nov 25, 2024

fivetran-joemarkiewicz Nov 22, 2024

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Nov 26, 2024

fivetran-reneeli Nov 26, 2024

fivetran-joemarkiewicz Nov 26, 2024

fivetran-reneeli Nov 26, 2024

fivetran-joemarkiewicz left a comment

fivetran-catfritz left a comment

fivetran-catfritz Nov 26, 2024

fivetran-reneeli Nov 26, 2024

Bugfix/redshift constant expressions #92

Bugfix/redshift constant expressions #92

Conversation

fivetran-reneeli commented Nov 20, 2024 • edited Loading

PR Overview

Under the Hood

PR Checklist

Basic Validation

Detailed Validation

If you had to summarize this PR in an emoji, which would it be?

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-catfritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-reneeli commented Nov 20, 2024 •

edited

Loading