-
Notifications
You must be signed in to change notification settings - Fork 2
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 wip incremental model #1
Add wip incremental model #1
Conversation
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.
Overall structure and approach is good, nice to see how it all flows together. I haven't really had a chance to look at any actual data yet (partly an error with report table), but it all makes sense to go ahead with this approach.
I think we should have a working report table out the box, not including any spend info, but make it easyish to add that in by default. A few other bits here and there but SO much easier to understand and follow than the last one!
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.
Feedback :)
86b1e1e
to
bd075c7
Compare
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.
LGTM, probably!
bd075c7
to
c142932
Compare
ea26a14
into
release/snowplow_attribution/0.1.0
Opening this PR for an early review to make sure the structure and incremental logic is agreed on before I go ahead and complete / revise it, add the integration tests and make the changes on docs. Currently it is only tested for Snowflake.
Core logic:
paths_to_conversion
incremental table collects the latest data in one go (starting from a user defined start date calledsnowplow_attribution_start_date
) taking into account the last conversion event that was processed for incremental runsincremental_manifest
table exists, later we could add warnings to prevent such user errorschannel and campaign attributions
, both of which apply all the different attribution algorithms apart from shapely (need to think about how to support it without python in a future release)report table
is currently set as a user configured one with a sample output, this is to allow for custom spend data integration and other flexibility such as including the currently optionalpaths_to_non_conversion
tables which for simplicity's sake is a drop and recompute table for nowpath summary and channel counts
tables for now but I'm not too sure they are needed for anything, we can agree with customers, we might add it as optional models if still useful, not sure