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.

Sign up for GitHub

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

Jump to bottom

462 improving uniqueness management in multi table schemas #484

Merged
merged 14 commits into from
Dec 20, 2024
Merged

462 improving uniqueness management in multi table schemas #484

merged 14 commits into from
Dec 20, 2024

Conversation

marcboulle
Copy link
Collaborator

@marcboulle marcboulle commented Dec 6, 2024 •
edited
Loading

Gestion des clés uniques dans les drivers de fichier et les base

  • conditionné par la méthode IsUnique de KWClass (auraravant GetRoot)
  • uniformisation de la terminologie, en passant de root a main, principalement pour la gestion des clés
    • dans les noms de méthodes (rappelé dans les notes de commit)
    • dans les noms d'attributs, de paramètres ou de variables locales
    • dans les commentaires (racine -> principal)

Les commits de type "Improve terminology between root and main in..." ne concernent que du renommage,
sans aucun impact sur le code:

  • ils peuvent être survolés lors du review
  • ils pourraient être fusionnés en un seul commit

Noveau test dédié sur LearningTest\TestKhiops\MultiTables\NoRootSnowflakeDuplicates
Tests complet sur tout LearningTest

Création d'un issue à discuter: #483

@marcboulle marcboulle requested a review from popescu-v December 6, 2024 14:57
@marcboulle marcboulle linked an issue Dec 6, 2024 that may be closed by this pull request
Improving uniqueness management in multi-table schemas #462
Closed
@marcboulle marcboulle force-pushed the 462-improving-uniqueness-management-in-multi-table-schemas branch 2 times, most recently from e3ac126 to 8fc4f20 Compare December 9, 2024 16:09
@marcboulle
Initialize management of uniqueness in multi-table schema
0921f5a 
// Unicite des instances de la classe
// - soit la classe est racine
// - soit elle contient des attributs de type relation non calcules, ce qui implique une unicite
//   de ses instances pour que chaque enregistrement de sous-table soit rattache de facon
//   unique a son enregistrement parent dans le schema multi-table hierachique
// L'unicite est controlee uniquement au moment de la lecture des donnes a partir d'une base
// pour des dictionnaire ayant des cles. Cela ne concerne pas les dictionnaires avec ou sans cle
// utilise pour la construction de table en memoire
// Cette caracteristique est calculee au moment de l'indexation de la classe
KWClass::IsUnique

Impacts principaux de la gestion de l'unicite
- KWClass
  - Set|GetForceUnique: methode protected avancee pour forcer l'unicite, meme pour une classe sans sous-tables
- KWDatabase
  - BuildPhysicalClass: memorisation par SetForceUnique de l'unicite de la classe physique,
    meme si ses attributs relation non calcules sont supprimes, car non utilises
- KWMTDatabase
  - DMTMPhysicalRead: gestion de l'unicite lors de la lecture selon le caractere IsUnique (et non GetRoot) de la classe

Quelques typos ou details corriges par ailleur
- KWDataPreparationTask::ComputeNecessaryClassAttributeMemory
- KWMTDatabase.h
- Standard.cpp
- Ermgt.cpp

WIP
@marcboulle
Save ForceUnique in KWClass using private meta-data
22cdcc5 
Gestion du ForceUnique de la classe pour la lecture/ecriture de dictionnaire dans les fichiers
Permet de transferer cette information "privee", par exemple pour une tache parallele

On en profite pour normaliser la terminologie des methodes de gestion de ces meta-donnees prives.

Impacts:
- KWClass
  - WritePrivateMetaData, ReadPrivateMetaData:
- KWAttribute
  - WritePrivateMetaData, ReadPrivateMetaData:

KWCLex: supression de caracteres accentues d'un commentaire
@marcboulle
Rename rootMultiTableMapping into mainMultiTableMapping in KWMTDatabase
a320285 
Ce renommage permet de clarifier la difference entre classe Root, utile pour la gestion des table externes,
et classe principale, la classe d'analyse qui n'est pas necessairement Root

Cette confusion existait dans le nom de la variable rootMultiTableMapping, avec nombreux impacts dans:
- KWMTDatabase
- PLMTDatabaseTextFile
- KWMTDatabaseStream

On fait un commit a part pour ne pas perturber l'analyse des autres commit
@marcboulle
Fix erroneous management of non-root main dictionary as a self-refere…
d7f6709 
…nced table

Contexte:
Les tables externes sont gerees par la le referenceSolver (classe KWObjectReferenceResolver),
pour retrouver les objets Root references.
Dans le cas particulier d'un objet principal Root, celui ci est enregitre temporairement dans le referenceSolver
pour gerer ses reference internes.
C'est le cas par exemple pour le dictionnaire Molecules, pour chainer ses atomes et liaisons
avec des references internes au flocon d'une molecule.

Le bug etait que l'objet principal etait enregistre des qu'il avait des champs cles, alors qu'il ne doit etre enregistre
que s'il est Root

Impacts:
- KWMTDatabase
  - PhysicalReadAllReferenceObjects
  - MutatePhysicalObject
  - IsPhysicalObjectSelected
@marcboulle
Improve terminology between root and main dictionary in KWMTDatabase …
c0bb843 
…and KWClassDomain

Il a souvent dans le code une confusion entre:
- main dictionary: dictionnaire d'analyse principal, pas necessairement Root
- root dictionary: dictionnaire racine identifie de facon unique, pour gerer les tables externes

Recherche systematique des utilisations de root dans le code, pour clarifier la terminologie
- renommage des methodes concernees si necessaire
  - on passe principalement de RootClass a MainClass (Class signifie dictionaire en terminologie interne)
  - renommage indique car -> dans les notes de commit
- renommage des parametres et des variable locales concernes
- renommage egalement des commentaires (racine -> principal)

Impacts
- KWMTDatabase
  - CheckPartially
  - CheckObjectConsistency
- KWClassDomain
  - WriteFileFromClass
  - CloneFromClass
  - ComputeClassDependence
  - GetClassDependanceUsedMemory
@marcboulle
Improve terminology between root and main dictionary in KWMTDatabaseM…
8891ca9 
…apping

- KWMTDatabaseMapping
  - GetDataRoot: supression de cette methode obsolete, jamais utilisee
@marcboulle
Improve terminology between root and main dictionary in KWDatabaseInd…
a35852e 
…exer

- KWDatabaseIndexer
  - GetChunkPreviousRootKeyAt -> GetChunkPreviousMainKeyAt
  - ComputeRootTableBasicIndexation -> ComputeMainTableBasicIndexation
  - ComputeRootTableIndexation -> ComputeMainTableIndexation
  - ExtractRootSampleKeyPositions -> ExtractMainSampleKeyPositions
@marcboulle
Improve terminology between root and main dictionary in KWDatabaseChu…
c298f91 
…nkBuilder

- KWDatabaseChunkBuilder
  - GetChunkLastRootKeyAt -> GetChunkLastMainKeyAt
@marcboulle
Improve terminology between root and main dictionary in KWArtificialD…
6d8ed95 
…ataset

- KWArtificialDataset
  - CreateDataset
@marcboulle
Improve terminology between root and main dictionary in KWKeyPosition…
6079136 
…FinderTask

- KWKeyPositionFinderTask
  - TestWithArtificialRootAndSecondaryTables -> TestWithArtificialMainAndSecondaryTables
@marcboulle
Improve terminology between root and main dictionary in KWTestDatabas…
6b6e68d 
…eTransfer

- KWTestDatabaseTransfer
  - MTTest
  - MTTestWithArtificialDatabase
  - MTMainTestReadWrite
  - MTTestReadWrite
@marcboulle
Improve terminology between root and main dictionary in KWPredictorEv…
4f936ec 
…aluator

- KWPredictorEvaluator
  - RenameDatabaseClasses
@marcboulle
Improve terminology between root and main dictionary in "Build multi-…
f6dbf56 
…table dictionary"

Cette fonctionnalite de la boite de dialogue "Data table key extractor" permet de construire
un schéma en etoile a partir d'une table de log, avec un table principale ayant des identifiant uniques.
L'impllementation actuel cree un dictionnaire principal, non Root, donc conforme a la nouvelle terminologie.
Par contre, l'aide dans les info-bulle reference la a tort notion de root dictionary, au lieuy de main dictionary

Corrections dans les info-bulles
- KWLearningProblemActionView
- KWDataTableKeyExtractorView
- KWMTClassBuilderView
@marcboulle marcboulle force-pushed the 462-improving-uniqueness-management-in-multi-table-schemas branch from 8fc4f20 to 54788bd Compare December 16, 2024 08:46
@marcboulle marcboulle mentioned this pull request Dec 16, 2024
Merged
popescu-v
popescu-v reviewed Dec 16, 2024
View reviewed changes
Copy link
Collaborator

@popescu-v popescu-v left a comment

Choose a reason for hiding this comment

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

A few questions and typo correction suggestions.

@marcboulle marcboulle force-pushed the 462-improving-uniqueness-management-in-multi-table-schemas branch from 54788bd to e06fbb2 Compare December 17, 2024 09:14
@marcboulle marcboulle requested a review from popescu-v December 17, 2024 09:14
popescu-v
popescu-v requested changes Dec 17, 2024
View reviewed changes
Copy link
Collaborator

@popescu-v popescu-v left a comment

Choose a reason for hiding this comment

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

See pending comments.

@marcboulle marcboulle requested a review from popescu-v December 17, 2024 13:06
@marcboulle
Finalize management of uniqueness in data table drivers and databases
a7d8ba4 
Gestion des cles uniques dans les drivers de fichier et les bases de donnes
- conditionne par la methode IsUnique de KWClass (auraravant GetRoot)
- uniformisation de la terminologie, en passant de root a main, principalement pour la gestion des cles
  - dans les noms de methodes (rappele dans les notes de commit)
  - dans les noms d'attributs, de parametres ou de variables locales
  - dans les commentaires (racine -> principal)

Impacts principaux
- KWMTDatabase
  - PhysicalSkip: conditionnements sur le IsUnique au lieu de GetRoot
- KWDatabaseTask
  - input_ChunkLastRootKey -> input_ChunkLastMainKey
- PLMTDatabaseTextFile
  - SetLastReadRootKey -> SetLastReadMainKey
- KWDataTableDriver
  - GetLastReadRootKey -> GetLastReadMainKey
- KWDataTableDriverTextFile
  - SkipRootRecord -> SkipMainRecord
  - ivRootKeyIndexes -> ivMainKeyIndexes
  - nombreux conditionnements sur le IsUnique au lieu de GetRoot des dictionnaires
- PLDataTableDriverTextFile
  - GetConstRootKeyIndexes -> GetConstMainKeyIndexes
  - GetRootKeyIndexes -> GetMainKeyIndexes

Autres impacts mineurs, uniquement dans les commentaires

Ajout du jeu de test: LearningTest\TestKhiops\MultiTables\NoRootSnowflakeDuplicates
- test avec schema en flocon, sans dictionnaire Root, et doublons dans les tables principale et secondaires
- verification que les doublons sont correctement detectees

Autres tests effectues a la main, sur des cas plus volumineux, ou avec une seule table avec cle, en mode Root ou non

Tests complets sur LearningTest

Les commits de type "Improve terminology between root and main in..." ne concernent que du renommage,
sans aucun impact sur le code:
- ils peuvent etre survoles lors du review
- ils pourraient etre fusionnes en un seul commit
@marcboulle marcboulle force-pushed the 462-improving-uniqueness-management-in-multi-table-schemas branch from e06fbb2 to a7d8ba4 Compare December 17, 2024 13:19
popescu-v
popescu-v approved these changes Dec 18, 2024
View reviewed changes
Copy link
Collaborator

@popescu-v popescu-v left a comment

Choose a reason for hiding this comment

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

LGTM

@marcboulle marcboulle merged commit 9367755 into dev Dec 20, 2024
23 checks passed
@marcboulle marcboulle deleted the 462-improving-uniqueness-management-in-multi-table-schemas branch December 20, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@popescu-v popescu-v popescu-v approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improving uniqueness management in multi-table schemas
2 participants
@marcboulle @popescu-v