-
Notifications
You must be signed in to change notification settings - Fork 45
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
Fix timing perfromance and user-agent models #41
Fix timing perfromance and user-agent models #41
Conversation
@sphinks I just kicked off the tests and it looks like everything is passing. That's probably because we're not currently testing these "optional" models though 🙃 This change may break existing projects, but I think it's good to be consistent with Snowplow's field names. Let me give this a quick spin locally to make sure we're in good shape, and then I think we can merge this! |
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 gave this another look with some fresh eyes and have a couple of quick questions!
@@ -17,7 +17,7 @@ web_page_context as ( | |||
prep as ( | |||
|
|||
select | |||
wp.page_view_id, | |||
wp.id, |
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.
hmm, does the web_page_context
model have a field called id
? I think at present, web_page_context
exposes two fields:
- root_id
- page_view_id
Does this work?
@@ -18,7 +18,7 @@ prep AS ( | |||
|
|||
SELECT | |||
|
|||
wp.page_view_id, | |||
wp.id, |
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.
same here -- I don't believe there's an id
field in the snowplow_base_web_page_context model?
Gosh... this code is kind of a mess.... So these optional models select directly from the web_page_context base model. I get it. I think that we can indeed select the
Curious to hear which of these approaches you think is preferable |
Let me a dig a little deeper, because I check at my side and for sure I get an error:
in case of original code. We'll take a look on you comment, to understand the rootcause. |
Cool, thanks @sphinks! I appreciate your help and insight here :) Let me know what you find! |
…ssed web page context to avoid code duplication, revert field renaming
@drewbanin Have reverted rename of fields and switch referencing from base model to preprocessed web context model, to reuse de-duplication and all the rest code. Please, take a look. |
Thanks @sphinks, this LGTM! Will merge this today |
@drewbanin Great! |
Fixing issue #40