-
Notifications
You must be signed in to change notification settings - Fork 0
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-98] Add Trending Snapshots DAG #65
Conversation
SELECT | ||
pp.PROJECT_ID, | ||
COUNT(DISTINCT rd.USER_ID) AS N_UNIQUE_USERS, | ||
COALESCE(TO_CHAR(MAX(rd.RECORD_DATE), 'YYYY-MM-DD'), 'N/A') AS LAST_DOWNLOAD_DATE, |
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.
In the event that we have no new downloads within the requested time window for a given project, we LEFT JOIN
onto PUBLIC_PROJECTS
and fill NULL
values with N/A
.
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 your rationale here for shifting this to a character? the expected output is what we have for "export_date" so keep that in mind. Nice use of Coalese though for those projects that don't have download dates at all.
One query optimization is to first get the top 10 downloaded public projects by user, then only grab the data sizes and activity for those projects.
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 your rationale here for shifting this to a character?
It's because COALESCE
needs all the arguments to be the same data type (in this case, string). I was getting a Date 'N/A' is not recognized
error otherwise.
the expected output is what we have for "export_date" so keep that in mind
Noted - it looks like the format on Synapse is determined by how the schema is formatted on Synapse itself. I did a test run using the date formatting as it is ('YYYY-MM-DD'
) but it shows up like MM/DD/YYYY
on Synapse, same formatting as export_date
:
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.
One query optimization is to first get the top 10 downloaded public projects by user, then only grab the data sizes and activity for those projects.
RECENT_DOWNLOADS
still works with PUBLIC_PROJECTS
(not TOP_10
) because the downloads are what's used to determine what the top 10 projects for that month are. After that, the remaining CTEs use the results from the TOP_10
project CTE to get the file handles and the corresponding total size for each project, which minimizes calculations.
Here are my results for the month of June:
![image](https://private-user-images.githubusercontent.com/32107699/354664907-1cf4c2ff-b8fc-4625-a184-a76cdfaa65e2.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQxMzEyOTksIm5iZiI6MTcyNDEzMDk5OSwicGF0aCI6Ii8zMjEwNzY5OS8zNTQ2NjQ5MDctMWNmNGMyZmYtYjhmYy00NjI1LWExODQtYTc2Y2RmYWE2NWUyLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODIwVDA1MTYzOVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTNhNzY4ODE1MTdhZTMwOTliZWNhN2QxNDAxMGMzOTViMmNmZGRiNmIyYjIzNmRkZWZjZWVhNDFjYjNlMGZiZjAmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.xYdpU695JlgwppVOl8X653AkNxUHkNxhDKN0HRiK0aU)
I tried comparing the Query duration between the old and new version of this query, but tbh the calculation time is so small it's hard to tell. I think logically, though, this change does minimize compute.
One other thing that came to mind as I was working on this is we should consider expanding the scope of the trending project snapshots to also include upload activity. In other words N_UNIQUE_USERS
would include users both uploading and downloading, and we'd have another column for LAST_UPLOAD_DATE
.
SELECT DISTINCT ID, PROJECT_ID, FILE_HANDLE_ID | ||
FROM SYNAPSE_DATA_WAREHOUSE.SYNAPSE.NODE_LATEST | ||
WHERE 1=1 | ||
AND NODE_TYPE = 'file' | ||
AND PROJECT_ID IN (SELECT PROJECT_ID FROM PUBLIC_PROJECTS) |
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.
NIt: I was going to say you could combine the PUBLIC_PROJECTS / LATEST_FILE_HANDLES / FILE_SIZES into one CTE, but I understand why you have PUBLIC_PROJECTS as a separate CTE.
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.
While I was working on the optimization suggestion from this thread, I considered having the RECENT_DOWNLOADS
CTE join FILEDOWNLOAD
and NODE_LATEST
and getting rid of PUBLIC_PROJECTS
altogether, but I decided against it because:
- I don't believe it would minimize compute ( I tried it out a few times and found no significant difference in Query duration between the version with
PUBLIC_PROJECTS
) - Smaller CTEs are easier to read and makes the query easier to understand (from my perspective)
But lmk what you think!
FROM SYNAPSE_DATA_WAREHOUSE.SYNAPSE.NODE_LATEST | ||
WHERE 1=1 | ||
AND NODE_TYPE = 'file' | ||
AND PROJECT_ID IN (SELECT PROJECT_ID FROM TOP_10_PUBLIC_PROJECTS) |
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.
We should probably add in a is_public
check here as well because there can be private files within a public project that we want to filter activity out for.
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 decided against this because calculating a project size based off only the public files wouldn't truly reflect the size of the project itself. It would also mislead users into thinking that they have access to all of a project, when in reality they might only have access to a portion of it, if that makes sense. So right now I'm still leaning against filtering for public files. What do you think?
JOIN FILE_SIZES | ||
ON TOP_10_PUBLIC_PROJECTS.PROJECT_ID = FILE_SIZES.PROJECT_ID | ||
|
||
GROUP BY |
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.
Just some food for thought as there are many ways to solve a problem. Another approach is to do this in two parts
- Calculate the top 10 downloaded projects and their last download activity (
DL_INFO_CTE
) - Calculate the data size of those top 10 downloaded project (
DATA_SIZE_CTE
) - join
DL_INFO_CTE
withDATA_SIZE_CTE
onproject_id
column.
That's effectively what you're doing here, but this group by
clause can be a bit confusing at first glance
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! I'm going to pre-approve, but please see some of my comments. Excellent work here!
Thanks for the review @thomasyu888! I'll wait until after Sage Week before merging. Some final to-do items for me:
Quick thought: Thanks! |
Great work here! Yes, please run it manually this time, and September will be automated. |
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.
1 comment around export date. But everything else looks good. I will go ahead and pre-approve, please re-request review if there are sizable changes.
Before I merge this in, I'm having second thoughts about what's in The screenshot below shows a backfill of the top 10 trending Projects in February 2024. While I think the important metric is the number of unique users, as that is what determines what the "top 10" trending projects are, but I wonder if that project size column is misleading. If we want
But I'd like to know your thoughts on this before I move forward @BryanFauble @thomasyu888 ![]() The other solution (simpler) is just change the column name to |
@jaymedina this is a great catch! I think what we probably care about is when the entity was created on, so if we filter out all entities created before the runtime, it would give us a more accurate representation. The issue with this is that we don't capture deletions so the data won't be fully accurate. |
I see. Then I'll move forward with modifying the total project size calculations and can fold the caveat you mention into the column name itself by modifying the name to be |
Great idea - before doing that change, let's let the frontend team know we are going to make a schema change. Can you post on #synapse? This will impact the homepage. |
After Jay's feedback I've moved forward with the schema modifications and calculating the Project size based off the files available in Here is a test I just ran with the newest DAG that updates the table for July 2024 and December 2022. Take a look at the new Project size column and notice the difference between Project sizes for the same Projects (notably: I'm re-requesting a review since this is a structural change. Thank you both for your patience and for your feedback! |
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.
Great change!
SELECT ID, PROJECT_ID, FILE_HANDLE_ID | ||
FROM SYNAPSE_DATA_WAREHOUSE.SYNAPSE.NODE_LATEST | ||
WHERE 1=1 | ||
AND DATE_TRUNC('MONTH', CHANGE_TIMESTAMP) <= DATE_TRUNC('MONTH', DATE('{context["params"]["month_to_run"]}')) |
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 will be an issue because sometimes the CHANGE_TIMESTAMP will be greater than the CREATEDON date. Let's say an entity was created in Feb but updated in April, by doing this query, when you do the calculation of data sizes for Feb, you're going to leave this entity out.
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.
Got it, I've updated it so that as long as the file was created <= the month_to_run
, it will be used in the calculation of the total Project size.
|
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!
This DAG will automate populating the Trending Projects Snapshots table at the users' desired cadence.
TOTAL_DATA_SIZE_IN_GIB
testing
1 : Test query efficacy
Testing the query on this test Project ensures that the project size (i.e. the total number of File entities stored within a project) is being calculated correctly:
2 : Test query efficacy
Confirming the query results for the 30 days leading up to 6/29/2024 approach the results from the initial query
** Current query results **
![image](https://private-user-images.githubusercontent.com/32107699/354412984-10077af6-0cfd-4dcb-9aa4-d03187b1b60e.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQxMzEyOTgsIm5iZiI6MTcyNDEzMDk5OCwicGF0aCI6Ii8zMjEwNzY5OS8zNTQ0MTI5ODQtMTAwNzdhZjYtMGNmZC00ZGNiLTlhYTQtZDAzMTg3YjFiNjBlLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODIwVDA1MTYzOFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTEyNThiYzc3YjkyNmI1N2ZjMDNjY2Y4MzIzOTM2MjgyOGQxMzA4NjBhMTEwZGFlODkyYTU4ZGYxYTVjZTExMTMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.hoCJFneCs4k-KKnE68TjraZ6KgEELxqT_Q252RAnGZg)
** Initial query results **
![image](https://private-user-images.githubusercontent.com/32107699/354412789-2847ae04-5b78-42bd-9e94-7badeb6dd784.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQxMzEyOTgsIm5iZiI6MTcyNDEzMDk5OCwicGF0aCI6Ii8zMjEwNzY5OS8zNTQ0MTI3ODktMjg0N2FlMDQtNWI3OC00MmJkLTllOTQtN2JhZGViNmRkNzg0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODIwVDA1MTYzOFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTc0ZDUyYTFlMWIwZTJlMTYwYTVmMDZlNjhiOWRhZjcyMDcwMDg5MTExODY4NTNhYmM5N2Q1MWM4MGYwZTZkYTkmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.4qqn_AmoHt5mKTIlAdd5bXWaBIODpqPi4nrOddH9Clk)
3 : Test query efficacy
This query also accounts for cases where there are no new downloads / users within the time range specified (thanks to
LEFT JOIN
)4 : Test DAG functionality
Trending Snapshots Test
5 : Test DAG functionality
I changed
time_window
in thedag_config
to 2 days to ensure then_unique_users
reflects this shorter time window. Results:Trending Snapshots Test 2 days