extract_drug_dispenses.R : correction requêtage tables archivées#83
Open
MarcDibling wants to merge 1 commit intoSNDStoolers:mainfrom
Open
extract_drug_dispenses.R : correction requêtage tables archivées#83MarcDibling wants to merge 1 commit intoSNDStoolers:mainfrom
MarcDibling wants to merge 1 commit intoSNDStoolers:mainfrom
Conversation
strayMat
reviewed
Mar 26, 2026
Collaborator
strayMat
left a comment
strayMat
left a comment
There was a problem hiding this comment.
Merci beaucoup pour la contirbution !
J'ai fait une mini suggestion de forme mais sinon le code me semble bien. Je ne vois pas facilement de manière d'éviter cette complexité induite par les tables archivés en gardant la structure de requête par flux...
En revanche, j'ai vu dans la CI que deux tests semblaient fail : Je n'arrive pas à voir lesquels mais il faudrait vérifier que les test passent.
Sinon c'est bon pour merger.
| if (year <= first_non_archived_year && month %in% c(1:2)) { | ||
| years_to_query <- c(year - 1L, year) | ||
| } else { | ||
| years_to_query <- year |
Collaborator
There was a problem hiding this comment.
Ca a l'air de marcher de mettre un scalaire mais j'aurais préféré garder un vecteur c(year) ici.
| } | ||
|
|
||
| query <- query |> dbplyr::sql_render() | ||
| if (!dbExistsTable(conn, output_table_name)) { |
Collaborator
There was a problem hiding this comment.
good, c'est mieux que ce que l'on a :)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Correction issue #82:
On retrouve donc des executions de décembre de l’année Y-1 dans les tables de l’année Y-1 mais aussi dans celles de l’année Y. Ainsi, si on ne requête pas pour les flux de janvier et février en Y ET en Y-1 on perd de l’exhaustivité.
Pour apporter le moins possible de modifications à l’architecture du code actuel, je propose de migrer la définition des tables à requête dans la boucle qui les requête mois par mois et ajouter une troisième boucle visant à requêter les tables Y et Y-1 pour un même flux seulement pour les mois de flux de janvier à février.
Je vous propose donc une PR qui complique légèrement la fonction mais qui garantit une exhaustivité.