Skip to content
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

[SNOW-155] Converting userprofile_latest table to dynamic table #100

Merged
merged 6 commits into from
Jan 11, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
-- Introduce the dynamic table
USE SCHEMA {{database_name}}.synapse; --noqa: JJ01,PRS,TMP
CREATE OR REPLACE DYNAMIC TABLE USERPROFILE_LATEST
(
CHANGE_TYPE VARCHAR(16777216) COMMENT 'The type of change that occurred to the user profile, e.g., CREATE, UPDATE (Snapshotting does not capture DELETE change).',
CHANGE_TIMESTAMP TIMESTAMP_NTZ(9) COMMENT 'The time when any change to the user profile was made (e.g. create or update).',
CHANGE_USER_ID NUMBER(38,0) COMMENT 'The unique identifier of the user who made the change to the user profile.',
SNAPSHOT_TIMESTAMP TIMESTAMP_NTZ(9) COMMENT 'The time when the snapshot was taken (It is usually after the change happened).',
ID NUMBER(38,0) COMMENT 'The unique identifier of the user.',
USER_NAME VARCHAR(16777216) COMMENT 'The Synapse username.',
FIRST_NAME VARCHAR(16777216) COMMENT 'The first name of the user.',
LAST_NAME VARCHAR(16777216) COMMENT 'The last name of the user.',
EMAIL VARCHAR(16777216) COMMENT 'The primary email of the user.',
SNAPSHOT_DATE DATE COMMENT 'The data is partitioned for fast and cost effective queries. The snapshot_timestamp field is converted into a date and stored in the snapshot_date field for partitioning. The date should be used as a condition (WHERE CLAUSE) in the queries.',
CREATED_ON TIMESTAMP_NTZ(9) COMMENT 'The creation time of the user profile.',
IS_TWO_FACTOR_AUTH_ENABLED BOOLEAN COMMENT 'Indicates if the user had two factor authentication enabled when the snapshot was captured.',
TOS_AGREEMENTS VARIANT COMMENT 'Contains the list of all the term of service that the user agreed to, with their agreed on date and version.',
LOCATION VARCHAR(16777216) COMMENT 'The location of the user.',
COMPANY VARCHAR(16777216) COMMENT 'The company where the user works.',
POSITION VARCHAR(16777216) COMMENT 'The position of the user in the company.',
INDUSTRY VARCHAR(16777216) COMMENT 'The industry/discipline that this person is associated with.'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if explicitly listing out the columns like this would ever clash with the SELECT * from the main query, if say userprofilesnapshot gets a column added to it later down the line? So either we keep the SELECT * and implicitly call the columns in this script + introduce the column comments in a separate script, or the columns selected are explicitly written out in the main query of this script

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're not modifying any columns from the upstream table I recommend using the more succinct style which you can see in Jenny's PR which I linked to below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I will add column comments as a separate step as like what Jenny said, new columns might be added to the source table.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a heads up: I noticed that dynamic table comments don't show up in their own little Description box the way regular table comments do. With the dynamic table, you have to dive into the Table Definition itself to see that the dynamic table comment was added. It would be nice if we can also have the dynamic table comments front and center the way it is with the normal ones, but for now this is what it is. Dynamic tables were just added last year, so I'm sure this is a QOL improvement they'll add in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the only ways to check the Table comments are either hover over the table name and the comment section which is pretty tiny or refer to the Table Definition. Hope the Description box gets added soon.

)
TARGET_LAG = '1 day'
WAREHOUSE = compute_xsmall
COMMENT='This dynamic table contain the latest snapshot of user-profiles during the past 14 days. Snapshots are taken when user profiles are created or modified. Note: Snapshots are also taken periodically and independently of the changes. The snapshot_timestamp records when the snapshot was taken.'
AS
WITH dedup_userprofile AS (
SELECT
*,
"row_number"()
OVER (
PARTITION BY ID
ORDER BY CHANGE_TIMESTAMP DESC, SNAPSHOT_TIMESTAMP DESC
)
AS N
FROM {{database_name}}.SYNAPSE_RAW.USERPROFILESNAPSHOT --noqa: TMP
WHERE
SNAPSHOT_DATE >= CURRENT_TIMESTAMP - INTERVAL '14 days'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be consistent and use the same day interval for reduplication of all snapshots?

Copy link
Contributor

@jaymedina jaymedina Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistency is a good idea. If we do this, we will have to go with INTERVAL '30 days' as that is the time window for node_latest and we can't make that one shorter.

Edit: Coming back to this, it's better to keep the interval as 14 days for all dynamic tables but with exceptions like node_latest because not all snapshot tables have the change_type=DELETE value, so we will inevitably end up with duplicates in all our tables if we stick to a single window (which would have to be 30+ days because of the exceptions)

Copy link
Contributor Author

@danlu1 danlu1 Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second Jenny. Another reason is that the majority of the existing dynamic table is implementing the 14-days interval. @jaymedina I will add the interval setting in SOP.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, thank you!

Copy link
Member

@thomasyu888 thomasyu888 Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaymedina Another risk that we assume is if snapshotting doesn't occur for 2 weeks in a row.

That said - I think that risk is small except around the holidays. We will need to determine all the tables that we need to be concerned about delete events (even if they don't all track that change type). So do all of these below need to use interval of 30?

  • Teams
  • Node
  • File
  • ACL
  • teammember?
  • project settings?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like

  • AR snapshots
  • User group
  • Userprofile

As well ( although User group and Userprofile don't capture deletions )

Another thought I just had is a lot of these tables are related to each other (file entities in node_latest should also be present in file_latest) sooo maybe for consistency as far as data availability, having all the same windows is better for the users. It would be nice to expedite having change_type=DELETE be available for all snapshots with a delete option on Synapse so we don't have to worry about the time window thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaymedina @thomasyu888 @philerooski
I did a brief check and the following snapshot tables have change_type=DELETE:

  • ACCESSREQUIREMENTSNAPSHOTS
  • ACLSNAPSHOTS
  • FILESNAPSHOTS
  • NODESNAPSHOTS
  • PROJECTSETTINGSNAPSHOTS

Do we agree to keep time window for dynamic tables derived from snopshots with change_type=DELETE to 30 days and other dynamic tables to 14 days?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! This workaround will do until platform team can address the missing change_type=DELETE for some snapshot tables. Thanks!

QUALIFY
N=1
)
SELECT
* exclude (N, LOCATION, COMPANY, POSITION, INDUSTRY),
NULLIF(LOCATION, '') AS LOCATION,
NULLIF(COMPANY, '') AS COMPANY,
NULLIF(POSITION, '') AS POSITION,
NULLIF(INDUSTRY, '') AS INDUSTRY,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we ever figure out why some columns were being recorded as null and others as a blank string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, not sure why this happened. Besides these columns, First_Name and Last_Name do have both empty strings and nulls. But I feel these 4 columns are more generic for business metrics so I convert empty strings for them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a platform ticket to investigate this here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bugged Marco about this in the Slack thread to see if we can figure out whether this is in the Parquet or if Snowflake is introducing these inconsistencies during data load.

@danlu1 if you want to merge this before we've figured out the root of the issue can you leave an inline comment with the Jira ticket identifier which Jenny linked so that we have some provenance for these changes?

Copy link
Contributor Author

@danlu1 danlu1 Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Marco's reply, it seems there is no easy answer for the discrepancies since he noticed this in Athena as well. I'd just merge the PR and leave a comment and we can revisit after SWC-7215 is resolved.

FROM
dedup_userprofile;
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
-- Backup the original latest table
USE SCHEMA {{database_name}}.synapse; --noqa: JJ01,PRS,TMP

-- Clone the USERPROFILE_LATEST table to ``USERPROFILE_LATEST_BACKUP`` for validation purposes
CREATE OR REPLACE TABLE USERPROFILE_LATEST_BACKUP CLONE USERPROFILE_LATEST;
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
-- Drop the ``USERPROFILE_LATEST`` table
USE SCHEMA {{database_name}}.synapse;
DROP TABLE IF EXISTS USERPROFILE_LATEST;
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
-- Drop the snapshot stream
USE SCHEMA {{database_name}}.synapse_raw;
DROP STREAM IF EXISTS USERPROFILESNAPSHOT_STREAM;
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
-- Drop any scheduled tasks
USE SCHEMA {{database_name}}.synapse_raw;
-- Suspend ROOT TASK
ALTER TASK IF EXISTS REFRESH_SYNAPSE_WAREHOUSE_S3_STAGE_TASK SUSPEND;
-- Drop LATEST_TABLE UPSERTING TASK
DROP TASK IF EXISTS UPSERT_TO_USERPROFILE_LATEST_TASK;
-- Resume the ROOT task and its child tasks
SELECT SYSTEM$TASK_DEPENDENTS_ENABLE( 'REFRESH_SYNAPSE_WAREHOUSE_S3_STAGE_TASK' );