Utilisation de fonction wrappers pour la création de tables à partir de requêtes

abelloir-zz commented 2 months ago

J'ai initialement créé la fonction wrapper create_table_from_query car je voulais pourvoir réaliser une étape du type :

query <- dbplyr::sql_render(query)
DBI::dbExecute(
    conn,
    glue::glue(
      "CREATE TABLE {output_table_name} AS {query}"
    )
  )

mais qui puisse marcher également pour mettre à jour une table donnée : par exemple créer une requête basée sur "MY_TABLE" à partir de laquelle on met à jour "MY_TABLE", ce qui implique créer une table temporaire, supprimer "MY_TABLE" et créer à nouveau "MY_TABLE" à partir de la table temporaire. D'où la création de la fonction suivante :

create_table_from_query <- function(conn = NULL,
                                    output_table_name = NULL,
                                    query = NULL,
                                    overwrite = FALSE) {
  if (DBI::dbExistsTable(conn, output_table_name)) {
    stopifnot(overwrite)
  }
  query <- dbplyr::sql_render(query)

  temp_table_name <- paste0(output_table_name, "_TMP")
  DBI::dbExecute(
    conn,
    glue::glue("CREATE TABLE {temp_table_name} AS {query}")
  )

  if (DBI::dbExistsTable(conn, output_table_name) && overwrite) {
    DBI::dbRemoveTable(conn, output_table_name)
  }

  DBI::dbExecute(
    conn,
    glue::glue(
      "CREATE TABLE {output_table_name} AS SELECT * FROM {temp_table_name}"
    )
  )
  DBI::dbRemoveTable(conn, temp_table_name)
}

A la réflexion, ce cas particulier n'est pas souhaitable donc la fonction create_table_from_query peut être largement épurée : je la mets à jour dans #34

Par ailleurs, le regroupement de CREATE TABLE et de INSERT INTO au sein d'une même fonction me parait finalement apporter plus de confusion que de concision. J'ai remplacé dans #34 create_table_or_insert_from_query par create_table_from_query et une nouvelle fonction insert_into_table_from_query.

Est ce que cela vous va si j'applique ce remplacement de create_table_or_insert_from_query par create_table_from_query et la nouvelle fonction insert_into_table_from_query pour l'ensemble des fonctions d'extraction concernées ?

@ThomasSoeiro proposait :

  DBI::dbExecute(conn, glue::glue("INSERT INTO {output_table_name} {query}"), overwrite = overwrite)

et

 DBI::dbExecute(conn, glue::glue("CREATE TABLE {output_table_name} AS {query}"), append = append)

Mais je ne crois pas que ces options overwrite et append existent dans dbExecute. Je propose de garder les deux fonctions wrapper create_table_from_query et insert_into_table_from_query même s'il ne s'agit que deux fois trois lignes de code : je trouve ça intéressant de garder la syntaxe SQL brut à part

Qu'est ce que vous en pensez ?

ThomasSoeiro commented 2 months ago

@ThomasSoeiro proposait :
  DBI::dbExecute(conn, glue::glue("INSERT INTO {output_table_name} {query}"), overwrite = overwrite)
et
 DBI::dbExecute(conn, glue::glue("CREATE TABLE {output_table_name} AS {query}"), append = append) 
Mais je ne crois pas que ces options overwrite et append existent dans dbExecute. Je propose de garder les deux fonctions wrapper create_table_from_query et insert_into_table_from_query même s'il ne s'agit que deux fois trois lignes de code : je trouve ça intéressant de garder la syntaxe SQL brut à part

Quelle nouille je suis, tu as raison ! (pour le reste je réponds quand j'y aurai réfléchi)

Une source d'inspiration : https://github.com/r-dbi/odbc/blob/455c937eb787cde8535b3ceac466e7b3c63f7efb/R/dbi-table.R#L33-L95

strayMat commented 2 months ago

Je pencherais pour éviter de se trimballer plusieurs fonctions surtout si on utilise ce schéma d'écriture dans plusieurs extracteurs. Ça risque d'être assez lourd et de rendre la logique snds moins bien lisible.

Le lien envoyé par thomas a l'air pas trop mal comme inspiration il me semble.

J'ai l'impression qu'il nous faut les arguments overwrite et append et je ne suis pas sûr d'avoir en tête s'il faut qu'on garde leur argument temporary.

Je regarde à tête reposée cette fin de semaine pour proposer une implémentation condensée (sauf si vous pensez que c'est vraiment mieux de garder les deux fonctions séparées).

Une autre source d'inspiration qui m'a l'air plus simple à copier pour notre usage : https://github.com/duckdb/duckdb-r/blob/main/R/dbWriteTable__duckdb_connection_character_data.frame.R

abelloir-zz commented 2 months ago

Je n'ai pas de préférence pour utiliser ou non plusieurs fonctions pour la création ou complétion de tables SQL. Juste une remarque : pour la fonction create_or_insert_table_from_query l'intérêt était d'avoir un comportement en interne qui détermine si on doit créer (CREATE TABLE) ou append (INSERT INTO) automatiquement. Rajouter un argument append n'a donc pas trop d'intérêt. Mais Thomas a raison en fait c'est vrai que l'on peut remplacer (par exemple dans extract_drug_dispenses) directement

create_table_or_insert_from_query(
    conn = conn, 
    output_table_name =output_table_name, 
    query = query
)

par

  if (DBI::dbExistsTable(conn, output_table_name)) {
      DBI::dbExecute(
        conn,
        glue::glue("INSERT INTO {output_table_name} {query}")
      )
  } else {
    DBI::dbExecute(
      conn,
      glue::glue("CREATE TABLE {output_table_name} AS {query}")
    )
  }

Puisque dans le code de cette fonction on a déjà géré en amont l'écrasage de la table existante si overwrite est TRUE (voir #34 ). En fait on fait déjà de la syntaxe SQL dans les filtres et c'est mieux de d'exposer les commandes SQL de base là où elles sont utilisées pour quelqu'un qui voudrait un peu regarder comment le code fonctionne.

strayMat commented 2 months ago

Il me semble que c'est plus clair d'avoir une seule fonction similaire à celle de duckdb ou de r-dbi, avec la signature suivante : write_table_from_query(con, output_table_name, query, overwrite = FALSE, append = FALSE). Cette fonction permet de gérer les différents cas où l'on veut écrire une table dans oracle à partir d'une query. Mais j'ai peut-être pas tous les cas en tête, n'hésitez pas à me dire si je loupe un truc.

Du coup le code suivant:

  if (DBI::dbExistsTable(conn, output_table_name)) {
      DBI::dbExecute(
        conn,
        glue::glue("INSERT INTO {output_table_name} {query}")
      )
  } else {
    DBI::dbExecute(
      conn,
      glue::glue("CREATE TABLE {output_table_name} AS {query}")
    )
  }

Me semble plus clairement remplacé par :

  write_table_from_query(con, output_table_name, query, append=TRUE)

En revanche, je ne suis pas sûr d'avoir compris ce passage : En fait on fait déjà de la syntaxe SQL dans les filtres et c'est mieux de d'exposer les commandes SQL de base là où elles sont utilisées pour quelqu'un qui voudrait un peu regarder comment le code fonctionne.

En vrai, c'est un peu du chipotage : le fond de ce qui me gêne est d'avoir une dizaine de lignes de codes qui ont trait à de la gestion de l'écriture des tables au lieu du métier sur l'extraction SNDS dans les fonctions d'extraction. Mais si ça vous pose pas de pb, on garde le if else avec les requêtes DBI sans les mettre dans un utilitaire :)

ThomasSoeiro commented 2 months ago

Je vote pour le if ... else ... explicite car :

C'est plus clair.
Il y a beaucoup de cas différents (itération ou pas, overwrite ou append.
La fonction nous pose plus de problèmes qu'elle n'en résout, on a eu besoin de la changer à chaque PR.

strayMat commented 2 months ago

Ok :) Je changerai ça dans une petite PR sur https://github.com/strayMat/sndsTools/blob/main/R/extract_hospital_consultations.R

strayMat / sndsTools

Utilisation de fonction wrappers pour la création de tables à partir de requêtes #37