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

feat(replay): viewed_by_id migration #5706

Merged
merged 7 commits into from
Mar 29, 2024
Merged

Conversation

aliu39
Copy link
Member

@aliu39 aliu39 commented Mar 29, 2024

@aliu39 aliu39 requested review from a team as code owners March 29, 2024 20:29
@aliu39 aliu39 requested a review from cmanallen March 29, 2024 20:29
Copy link
Member

@cmanallen cmanallen left a comment

Choose a reason for hiding this comment

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

Need to update group_loader.py.

@aliu39 aliu39 requested a review from cmanallen March 29, 2024 20:44
Copy link

This PR has a migration; here is the generated SQL

-- start migrations

-- forward migration replays : 0018_add_viewed_by_id_column
Local op: ALTER TABLE replays_local ADD COLUMN IF NOT EXISTS viewed_by_id UInt64 AFTER user_email;
Distributed op: ALTER TABLE replays_dist ADD COLUMN IF NOT EXISTS viewed_by_id UInt64 AFTER user_email;
-- end forward migration replays : 0018_add_viewed_by_id_column




-- backward migration replays : 0018_add_viewed_by_id_column
Distributed op: ALTER TABLE replays_dist DROP COLUMN IF EXISTS viewed_by_id;
Local op: ALTER TABLE replays_local DROP COLUMN IF EXISTS viewed_by_id;
-- end backward migration replays : 0018_add_viewed_by_id_column

Copy link

codecov bot commented Mar 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.87%. Comparing base (7b6d69b) to head (e76040a).
Report is 3 commits behind head on master.

✅ All tests successful. No failed tests found ☺️

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5706      +/-   ##
==========================================
+ Coverage   89.86%   89.87%   +0.01%     
==========================================
  Files         900      901       +1     
  Lines       43770    43781      +11     
  Branches      301      301              
==========================================
+ Hits        39332    39350      +18     
+ Misses       4396     4389       -7     
  Partials       42       42              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.



columns: List[Tuple[str, Column[Modifiers]]] = [
("user_email", Column("viewed_by_id", UInt(64)))
Copy link
Member

Choose a reason for hiding this comment

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

what does user_email have to do with anything here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the column that it comes after

Copy link
Member Author

Choose a reason for hiding this comment

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

Following the boilerplate we used in 0017

@volokluev
Copy link
Member

This looks good to me. You can make the check pass by moving the yaml changes into a different PR. This makes sure the column will be there before we start being able to query it

@aliu39 aliu39 enabled auto-merge (squash) March 29, 2024 21:11
@aliu39 aliu39 merged commit e988549 into master Mar 29, 2024
32 checks passed
@aliu39 aliu39 deleted the aliu/viewed-replay-migration branch March 29, 2024 21:40
@aliu39
Copy link
Member Author

aliu39 commented Apr 2, 2024

yaml changes moved to #5709

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants