-
Notifications
You must be signed in to change notification settings - Fork 1
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
PDE-5817 Add support for profiles/events #2
base: main
Are you sure you want to change the base?
Conversation
return metadata.to_list(mdata) | ||
|
||
def stream_is_selected(mdata): | ||
return mdata.get((), {}).get('selected', False) |
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.
what is the first argument ()
to mdata.get
?
a comment explaining that, or even better a comment showing the shape (or example) of mdata
, would be good.
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 is copied from this line of code and I have no idea what it means but it's keying into a structure that looks like:
"metadata": [
{
"breadcrumb": [],
"metadata": {
"table-key-properties": "uuid",
"selected": true
}
}
]
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.
After some investigating, it's the Singer's python library's way of basically specifying key-paths into a dict. ()
appears to be equivalent to keying into the top-level of fields in the dictionary. See "breadcrumb" here.
tap_klaviyo/__init__.py
Outdated
'full' | ||
) | ||
|
||
FULL_STREAMS = [GLOBAL_EXCLUSIONS, LISTS, LIST_MEMBERS, EVENTS, PROFILES] |
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.
If I remember correctly, FULL_STREAMS
refers to streams extracted in a full overwrite manner. Is that right? Or is it just the full list of streams? I don't think we should extract things like EVENTS
full overwrite -- can we not do it incrementally?
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 came from here and as far as I can tell, FULL_STREAMS
is only used in the discover
operation, so I don't think it actually drives anything related to full/incremental.
Yeah this code is really something.
Add new global exclusions stream that uses the profiles endpoint
Comment out the old streams so we don't discover them
* Don't need this in discover mode * add new metrics endpoint
* Don't need this in discover mode * add new metrics endpoint * Sort in ascending order
* Don't need this in discover mode * add new metrics endpoint * Sort in ascending order * add profile_id to result
This adds two streams for events and profiles. This was taken hotgluexyz-klaviyo, which is a fork of Cody's original Klaviyo tap. The new
profiles
andevents
streams should allow us to migrate all existing streams by building views over the top of the raw profile and event data in Snowflake.