Feat: add origins in update db script #1409
Conversation
MarceloRobert
force-pushed
the
feat/add-origins-in-populate-db
branch
from
a61c5ea to
164b4ea
Compare
| if self.related_data_only: | ||
| existing_issues_ids = set( | ||
| Issues.objects.using(self.dashboard_conn_name).values_list( | ||
| "id", flat=True | ||
| ) | ||
| ) | ||
|
|
||
| if len(existing_issues_ids) == 0: | ||
| return [] | ||
| issue_id_placeholders = ",".join(["%s"] * len(existing_issues_ids)) | ||
| related_condition = f"AND issue_id IN ({issue_id_placeholders})" |
There was a problem hiding this comment.
Besides the query, this code seems duplicated many times. What do you think?
There was a problem hiding this comment.
Can you refactor for a wrapper, maybe?
There was a problem hiding this comment.
I can see that I could use a function to replace
issue_id_placeholders = ",".join(["%s"] * len(existing_issues_ids))
related_condition = f"AND issue_id IN ({issue_id_placeholders})"
but the rest seems like they would need to stay. Are you thinking of something else or is that it?
There was a problem hiding this comment.
I left another comment about it
| self.origins: list[str] | ||
| self.origin_condition: str |
There was a problem hiding this comment.
Why not initialize or make Optional, the same as the other fields?
There was a problem hiding this comment.
I figured that I didn't really need to initialize them or use Optional if the first thing the command does is assigning those variables. Do you think it's better with assign or without it?
There was a problem hiding this comment.
start_interval, end_interval, and related_data_onlywere also assigned as the first thing, and you assigned them
Since we removed the foreign key constraints, we don't need to limit the selected data to the related ones. With this option we can select if we want to fetch only related items or not.
MarceloRobert
force-pushed
the
feat/add-origins-in-populate-db
branch
from
164b4ea to
e0fb298
Compare
| self.origins: list[str] | ||
| self.origin_condition: str |
There was a problem hiding this comment.
start_interval, end_interval, and related_data_onlywere also assigned as the first thing, and you assigned them
| if self.related_data_only: | ||
| checkout_ids = set( | ||
| ( | ||
| Checkouts.objects.using(self.dashboard_conn_name) | ||
| .filter( | ||
| field_timestamp__gte=self.start_timestamp, | ||
| field_timestamp__lte=self.end_timestamp, | ||
| ) | ||
| .values_list("id", flat=True) | ||
| ) | ||
| .values_list("id", flat=True) | ||
| ) | ||
| ) | ||
|
|
||
| if len(checkout_ids) == 0: | ||
| return [] | ||
| checkout_id_placeholders = ",".join(["%s"] * len(checkout_ids)) | ||
| if len(checkout_ids) == 0: | ||
| return [] | ||
| checkout_id_placeholders = ",".join(["%s"] * len(checkout_ids)) | ||
| related_condition = ( | ||
| f"AND builds.checkout_id IN ({checkout_id_placeholders})" | ||
| ) |
There was a problem hiding this comment.
You can create a method that returns a tuple of string and set.
There was a problem hiding this comment.
You will avoid duplicating code, just use as parameters an array for defining the ids, the field for related_condition (builds.checkout_id)
| if self.related_data_only: | ||
| existing_build_ids = set( | ||
| Builds.objects.using(self.dashboard_conn_name) | ||
| .filter( | ||
| field_timestamp__gte=self.start_timestamp, | ||
| field_timestamp__lte=self.end_timestamp, | ||
| ) | ||
| .values_list("id", flat=True) | ||
| ) | ||
| .values_list("id", flat=True) | ||
| ) | ||
|
|
||
| if len(existing_build_ids) == 0: | ||
| return [] | ||
| build_id_placeholders = ",".join(["%s"] * len(existing_build_ids)) | ||
| if len(existing_build_ids) == 0: | ||
| return [] | ||
| build_id_placeholders = ",".join(["%s"] * len(existing_build_ids)) | ||
| related_condition = f"AND build_id IN ({build_id_placeholders})" |
There was a problem hiding this comment.
this method can be used here
| if self.related_data_only: | ||
| existing_issues_ids = set( | ||
| Issues.objects.using(self.dashboard_conn_name).values_list( | ||
| "id", flat=True | ||
| ) | ||
| ) | ||
|
|
||
| if len(existing_issues_ids) == 0: | ||
| return [] | ||
| issue_id_placeholders = ",".join(["%s"] * len(existing_issues_ids)) | ||
| if len(existing_issues_ids) == 0: | ||
| return [] | ||
| issue_id_placeholders = ",".join(["%s"] * len(existing_issues_ids)) | ||
| related_condition = f"AND issue_id IN ({issue_id_placeholders})" |
Description
The update_db script that moves data from kcidb to a separate db can have an optional origins filter so that we can avoid querying unnecessary data. Not only that but the current script still tries to follow the foreign key constraint by limiting results to the ones that have the related data in the target db, but that should be optional.
Changes
How to test
Use the update_db command with whatever interval; test the arguments --related-data-only (when set, the command should update less items) and --origins. Combine the origins to check if the counting is right (you can also check in kcidb directly if you want).
Closes #1401