search

carnival-data / carnival

JVM property graph data unification framework
https://carnival-data.github.io/carnival/
GNU General Public License v3.0
7 stars 2 forks source link

Remove KeyType #20

Closed augustearth closed 3 years ago

augustearth commented 3 years ago

KeyType was important for Carnival v0 when there were complex data operations that consumed and produced data tables. With the introduction of the graph in Carnival v1, these operations are done in the graph. KeyType is no longer central and should be removed for simplicity's sake.

There is overlap here with secondaryIdFieldMap of GenericDataTable and MappedDataTable. Like KeyType, the secondaryIdFieldMap was important for data table operations. Because secondaryIdFieldMap depends on KeyType, it is not possible to remove KeyType without removing secondaryIdFieldMap.

augustearth commented 3 years ago

Affected Files

project filename
clinical EncounterGroup
clinical PatientGroup
clinical OmopVine
clinical EncounterGroupSpec
clinical FrequencyMatcher
clinical FrequencyMatcherSpec
core DataSetDescriptorGraphSpec
core CachingVineSpec
core VineSpec
util KeyType
util GenericDataTable
util MappedDataTable
util FeatureReport
util TabularReport
util GenericDataTableSpec
util MappedDataTableSpec
util FeatureReportSpec
util TabularReportSpec
augustearth commented 3 years ago
augustearth commented 3 years ago

It turns out that EncounterGroup and PatientGroup rely on KeyType for some core functionality. So, rather than remove KeyType completely, I moved it up to carnival.clinical.util and left it in use by PatientGroup and EncounterGroup. I had already removed it from FrequencyMatcher.... which I suspect is a class that will be rethought anyways.