Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add user passthrough column to users model and update dependencies #44
add user passthrough column to users model and update dependencies #44
Changes from all commits
3754e57
dfe785d
0f093c6
0fb90e7
0876ec0
4883d86
ae866fd
7f45498
260cc6a
e7a53a4
79b3551
aafbb89
0c63d7c
fda5788
e5cd253
e2c2724
9107b7f
9124cb3
e68d5c7
43e2e19
a7360a3
a48cb0e
627b705
ce3b634
16268d7
d30829b
50d8ee4
70615c9
069aef1
dea51ce
1119d79
aaba9f8
0a9b49e
ebc02f7
4d156b9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add more about the
experiment_id
updates (plus the new columns you're adding) in either a Breaking Change section, or in Bug Fixes below, whichever makes more sense. It looks like it's mostly related to theiterable__using_event_extension
section, but more context would be helpful since the logic introduced is non-intuitive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Avinash, I added more info in Under the Hood for
iterable__using_event_extension
and the relatedexperiment_id
fieldThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll also want to add in here that the uniqueness test has been updated to account for instances when
iterable__using_event_extension
is enabled/disabled. It would be good to add what the new surrogate fields are and which models they can be found in.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link--https://github.com/fivetran/dbt_iterable/blob/main/README.md#passing-through-additional-fields--does not work as it's in a collapsed section of the README. I'd remove lines 105-106 and test if the hyperlink works on your branch then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the collapsed block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add more details around the consistency tests you've created here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fivetran-reneeli You will need to add the var
iterable__using_event_extension
to the yml section in step 4.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to bump the README
dbt_iterable
version in lines 76-80, now that it's a breaking change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah thank you, updated
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got this error when trying to test this model in the integration_tests validation run:
This is likely because
experiment_id
now is being removed in the default version. It shouldn't be too difficult to test that it successfully ran though--can you try and document your process for a successful test here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this-- I reran the tests and see the errors now. Actually,
experiment_id
should be there as default so this was unexpected. This led me to looking into the prod/dev runs and actually the missing column ischannel_type
. Hmm. I will continuing looking into why this is happeningThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fivetran-reneeli Strange. Can you look in the upstream models and see if they are present there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Avinash for helping troubleshoot and realize it was due to a missing comma!