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

Table pour le calcul d'indicateurs hebdo des candidats en recherche active #347

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

laurinehu
Copy link
Contributor

**Carte Notion : **

Pourquoi ?

Pour permettre l'historisation des indicateurs de candidats en recherche active.

Checks

  • J'ai lancé le modèle ou seed sur un dump local (si pertinent)
  • J'ai ajouté des tests à mon code Python, ou des assertions DBT sur le modèle SQL
  • J'ai documenté ce modèle voire certains de ses champs (usage métier, tableau de bord, etc)

@laurinehu
Copy link
Contributor Author

laurinehu commented Aug 21, 2024

à discuter :

  • vue mensuelle car une vue hebdo serait trop grosse (de 1M à 4M de lignes environ) -> une autre solution est de calculer directement les indicateurs agrégés pour avoir moins de lignes
  • pourquoi est-ce qu'on backup toutes les colonnes dans le snapshot des candidats en recherche active ? on pourrait ne garder que les colonnes qui changent et l'id du candidat (gain de place) et passer par une jointure ensuite pour récupérer les caractéristiques du candidat
  • ces tables sont déjà grandes et vont viiiiite grandir, il faut qu'on voit ensemble ce qu'on met en place pour économiser de la place avant que tout explose.

Romain je te mets en review dès maintenant car tu étais intéressé, mais j'aimerais bien qu'on en discute tous ensemble au retour de Yannick !

@laurinehu laurinehu self-assigned this Aug 21, 2024
Copy link
Contributor

@rsebille rsebille left a comment

Choose a reason for hiding this comment

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

Je met mes réflexions avant de les oublier 😁.

tl;dr : A mon sens c'est un questionnement philosophique qu'il faut avoir, une fois celui-ci répondu la technique sera évidente et suivra tout naturellement.

vue mensuelle car une vue hebdo serait trop grosse (de 1M à 4M de lignes environ)

La granularité dépend du besoin, si mensuel est suffisant alors mensuel sinon plus fin et on optimisera [1] si nécessaire : premature optimization is the root of all evil.
Et vu qu'on pourra changer de granularité quand on veux car la donnée est gardée dans le snapshot il n'y a pas trop de problème à commencer par mensuel pour changer ensuite.

pourquoi est-ce qu'on backup toutes les colonnes dans le snapshot des candidats en recherche active ? on pourrait ne garder que les colonnes qui changent et l'id du candidat (gain de place) et passer par une jointure ensuite pour récupérer les caractéristiques du candidat

Là y a 2 écoles, les 2 sont valides et dépendent de ce que vous envisager d'avoir besoin :

  1. snapshot de toute les données car on veux pouvoir faire un nouvel indicateur dans x temps et donc profiter de tout l'historique. => Ça prend (beaucoup) plus de place mais en contrepartie ça permet de tout faire n'importe quand.
  2. snapshot uniquement les colonnes avec la donnée qui nous intéresse. => Moins de place, et besoin de temps pour créer l'historique après l'ajout d'un nouvel indicateur

ces tables sont déjà grandes et vont viiiiite grandir, il faut qu'on voit ensemble ce qu'on met en place pour économiser de la place avant que tout explose.

Même type de réponse :), quel est le besoin actuel et anticipé ?
Si on veux tout l'historique alors pas le choix, si en fait on veux que les X derniers temps alors on peux limiter la donnée utilisée, soit depuis les sources soit de notre coté.


[1] Le nombre de ligne est assez peu important et de toute manière on s'attend a avoir de grosse tables, par contre faudra effectivement avoir une discipline de ne prendre que ce dont on aurais besoin et d'utiliser des types de colonnes appropriées.

@@ -0,0 +1,15 @@
select
{{ pilo_star(ref('candidats_recherche_active_snapshot'), except=["dbt_valid_from", "dbt_valid_to"]) }},
Copy link
Contributor

Choose a reason for hiding this comment

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

De ce que je comprend de la doc [1], il y a 4 champs ajouté par DBT

Suggested change
{{ pilo_star(ref('candidats_recherche_active_snapshot'), except=["dbt_valid_from", "dbt_valid_to"]) }},
{{ pilo_star(ref('candidats_recherche_active_snapshot'), except=["dbt_valid_from", "dbt_valid_to", "dbt_scd_id", "dbt_updated_at"]) }},

[1] https://docs.getdbt.com/docs/build/snapshots#snapshot-meta-fields

Copy link
Contributor

Choose a reason for hiding this comment

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

A confirmer mais je pense que ce modèle pourrais être incrémental, ça permettrais d'avoir un temps d’exécution stable plutôt que croissant et éviterais de recalculer le passé inutilement vu qu'il ne peux pas changer.

Copy link
Contributor

@YannickPassa YannickPassa left a comment

Choose a reason for hiding this comment

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

Qques petits changements.
Tu as vérifié avec un fichier test que ton modèle incrémental marchait correctement ?

@@ -208,3 +208,6 @@ models:
- name: candidatures_inconnues_employeurs_orienteurs
description: >
Table permettant le suivi des SIAE qui ont orienté un candidat n'ayant jamais postulé chez elles, vers d'autres SIAE.
- name: candidats_recherche_active_histo
Copy link
Contributor

Choose a reason for hiding this comment

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

Pourquoi c dans daily pour les properties et weekly pour le script ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oopsie erreur, je le bouge dans weekly

else date_trunc('week', current_date)
end,
interval '1 week'
) as week_series
Copy link
Contributor

Choose a reason for hiding this comment

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

On est d'accord que tu veux bien du hebdo (le titre de ta PR dit mensuel) ?

Comment on lines +26 to +31
-- we only update rows that have changed
-- the previous state is no longer current if :
-- 1: dbt_valid_from is bigger than last week date (this line is the new state)
-- 2: dbt_valid_to is bigger than last week date (this line is the last state that is now finished)
where dbt_valid_from > date_trunc('week', current_date) - interval '1 week'
or dbt_valid_to > date_trunc('week', current_date) - interval '1 week'
Copy link
Contributor

Choose a reason for hiding this comment

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

J'ai pas testé donc je vais peut-être dire de la merde mais je pense qu'on va avoir des problèmes :).

Mise à jour des lignes existantes

Si tu veux des UPDATE il faut donner une unique key (un truc du genre ["id", "semaine_valide"]) sinon les lignes qui se ferment vont être dupliquée, on aura celles enregistrées alors qu'elle était ouverte puis celle au moment de la fermeture, et d'ailleurs je pense que les lignes des lignes ouvertes vont aussi être dupliquée mais à chaque lancement pour celles-ci, et je me demande ce qui va se passer quand on a plusieurs lignes pour la même semaine 🤔.

Clause incrémentale

Il faut plutôt utiliser la dernière valeur connue dans la table que current_date, sinon on va avoir des trucs chelou en fonction de quand (et si) on lance le modèle. En gros ici tu supposes que le modèle va être lancer exactement 1 fois toutes les semaines au même moment, ce qui ne sera jamais vraiment le cas.
Et pour un peu les mêmes raisons il faut sûrement plutôt utiliser >= que > pour ne pas rater les lignes qui se ferment et s'ouvrent pendant une semaine déjà connue.

@laurinehu laurinehu changed the title Table pour le calcul d'indicateurs mensuels des candidats en recherche active Table pour le calcul d'indicateurs hebdo des candidats en recherche active Oct 9, 2024
laurinehu and others added 3 commits October 9, 2024 11:21
Co-authored-by: Romain Sébille <20045330+rsebille@users.noreply.github.com>
Co-authored-by: Romain Sébille <20045330+rsebille@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants