-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat(ingest/snowflake): support for more operation types #8158
feat(ingest/snowflake): support for more operation types #8158
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.
lgtm
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.
Saw this just got merged, but a couple question
"MERGE": OperationTypeClass.CUSTOM, | ||
"COPY": OperationTypeClass.CUSTOM, | ||
"TRUNCATE_TABLE": OperationTypeClass.CUSTOM, |
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 see that putting CUSTOM
allows us to show the customOperationType
, but for other sources we try to label these:
(unity)
QueryStatementType.COPY: OperationTypeClass.INSERT,
QueryStatementType.UPDATE: OperationTypeClass.UPDATE,
QueryStatementType.MERGE: OperationTypeClass.UPDATE,
QueryStatementType.DELETE: OperationTypeClass.DELETE,
QueryStatementType.TRUNCATE: OperationTypeClass.DELETE,
(bigquery)
"MERGE": OperationTypeClass.UPDATE,
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.
MERGE is in fact UPDATE and/or DELETE and/or INSERT, so I was hesitant to mark it as UPDATE. CUSTOM operationType is not great to use. cc: @jjoyce0510 . We could add more operation types in model maybe ? I am proposing addition of more operation types to - MERGE, COPY, TRUNCATE respectively. Does that make sense ?
customOperationType=query_type | ||
if operation_type is OperationTypeClass.CUSTOM | ||
else None, |
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.
Is there harm in passing this if the operation type class is not CUSTOM
, for more info? I know we don't do this elsewhere in the code but could start 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.
hmm, I thought that myself as well. Then customOperationType just behaves more like "nativeOperationType"
. cc - @jjoyce0510 - can you confirm this one as well please ?
Checklist