Incremented Load Flow by AdityaKBhadragond14 · Pull Request #13 · DigitalHealthIntegration/ETL-German-FHIR-Core

AdityaKBhadragond14 · 2024-03-12T13:08:30Z

This PR fixes the incremental load flow in the ETL Pipeline.

Issues and fixes:

Constraint Issue:
a. fpk_visit_detail_preceding_visit_detail_id this constraint in the visit_detail table was causing problem because it links the visit_detail_id and the preceding_visit_detail_id in the visit_detail_id and causes problem while deleting or updating because the visit_detail id is being used some other entries because of the foreign key reference. To fix this issue added ON DELETE CASCADE to the constraint to make sure all the entries with the references will also be deleted along with that particular entry.
b. fpk_visit_occurrence_care_site_id, fpk_visit_detail_visit_occurrence_id, fpk_condition_occurrence_visit_occurrence_id, fpk_drug_exposure_visit_occurrence_id, fpk_measurement_visit_occurrence_id, fpk_observation_visit_occurrence_id, fpk_provider_care_site_id same for this constraints as well in the care_site table.
In the custom mappers OrganizationMapper, PractitionerMapper the incremental load flow was not being handled. Added the check to handle that as well.
Also in the CareSiteRepository, VisitDetailRepository and ProviderRepository added queries to delete the entries using fhir_logical_id.
Also removed the part of the code from ResourceOmopReferenceUtils.java class where the update was happening in the fhir gateway which is not required.
In TaskConfiguration added the stepPractitioner and stepPractitionerRole to the incremental load flow.

vbothe23 · 2024-04-15T13:25:47Z

src/main/java/org/miracum/etl/fhirtoomop/TaskConfiguration.java

        .next(stepProcessConsent)
        .next(stepProcessDiagnosticReport)
+            .next(stepProcessPractitioners)
+            .next(stepProcessPractitionerRole)


Please add step for Provenance resource as well.

The provenance step does not exist yet in the codebase. This can be done after merging the appointment mapper PR.

SatwikSShanbhag

Thanks for good Explanation of PR
left few comments have a look

SatwikSShanbhag · 2024-04-18T13:42:00Z

src/main/java/org/miracum/etl/fhirtoomop/listeners/OrganizationStepListener.java

@@ -40,25 +40,26 @@ public void beforeStep(StepExecution stepExecution) {
        if (bulkload.equals(Boolean.TRUE)) {
            log.info("========= Preparing OMOP DB for BulkLoad =========");
            truncateDb();


is there any reason to move this code inside BulkLoad as I can see even in incremental load we need these setters ?

I have not moved anything here. Its like this from beginning.

SatwikSShanbhag · 2024-04-18T13:43:05Z

src/main/java/org/miracum/etl/fhirtoomop/TaskConfiguration.java

        .next(stepProcessDiagnosticReport)
+            .next(stepProcessPractitioners)
+            .next(stepProcessPractitionerRole)


Suggested change

.next(stepProcessDiagnosticReport)

.next(stepProcessPractitioners)

.next(stepProcessPractitionerRole)

.next(stepProcessDiagnosticReport)

.next(stepProcessPractitioners)

.next(stepProcessPractitionerRole)

SatwikSShanbhag · 2024-04-18T13:45:01Z

src/main/java/org/miracum/etl/fhirtoomop/mapper/EncounterDepartmentCaseMapper.java

+          ResourceFhirReferenceUtils referenceUtils,
+          IFhirPath fhirPath,
+          Boolean bulkload,
+          DbMappings dbMappings, OmopRepository repositories) {


Suggested change

ResourceFhirReferenceUtils referenceUtils,

IFhirPath fhirPath,

Boolean bulkload,

DbMappings dbMappings, OmopRepository repositories) {

ResourceFhirReferenceUtils referenceUtils,

IFhirPath fhirPath,

Boolean bulkload,

DbMappings dbMappings,

OmopRepository repositories) {

SatwikSShanbhag · 2024-04-18T13:56:27Z

src/main/java/org/miracum/etl/fhirtoomop/mapper/EncounterDepartmentCaseMapper.java

+  private final OmopRepository repositories;
+


can we use EncounterDepartmentCaseMapperServiceImpl to manage repository operations.

SatwikSShanbhag · 2024-04-18T13:59:14Z

src/main/java/org/miracum/etl/fhirtoomop/mapper/EncounterDepartmentCaseMapper.java

      String departmentCaseLogicId, String departmentCaseLogicIdentifier) {
    if (!Strings.isNullOrEmpty(departmentCaseLogicId)) {
-      departmentCaseMapperService.deleteExistingDepartmentcaseByLogicalId(departmentCaseLogicId);
+      repositories.getVisitDetailRepository().deleteEntriesByFhirLogicalId(departmentCaseLogicId);


define a delete function inside EncounterDepartmentCaseMapperServiceImpl and use this operation there

SatwikSShanbhag · 2024-04-18T14:07:19Z

src/main/java/org/miracum/etl/fhirtoomop/mapper/PractitionerMapper.java

    private final Boolean bulkload;
    private final DbMappings dbMappings;
-
+    private final OmopRepository repositories;


Same create a serviceImpl class and add repository operations there

SatwikSShanbhag · 2024-04-18T14:09:54Z

src/main/java/org/miracum/etl/fhirtoomop/mapper/helpers/ResourceOmopReferenceUtils.java

+//      readerTemplate.update(
+//          String.format("UPDATE %s SET last_updated_at=? WHERE fhir_id=?", inputTableName),
+//          LocalDateTime.now().plusDays(1),
+//          resourceId.substring(4));


can you please explain more on what was happening before and why is this now commented?

My bad I had commented it out by mistake.

according to discussion we had this was commented before only. but here I can see we are commenting it
please explain more on this. pipeline id failing if we keep this piece of code.

SatwikSShanbhag · 2024-04-18T14:14:55Z

src/main/java/org/miracum/etl/fhirtoomop/repository/CareSiteRepository.java

+  @Transactional
+  @Modifying
+  @Query(value = "DELETE FROM care_Site WHERE fhir_logical_id = :fhir_logical_id", nativeQuery = true)
+  void deleteCareSiteByLogicalId(@Param("fhir_logical_id") String fhirLogicalId);


you can use directly void deleteByFhirLogicalId(String fhirLogicalId); no need to add query, try this approach .

use void deleteByFhirLogicalId(String fhirLogicalId); its available by default

SatwikSShanbhag · 2024-04-18T14:22:52Z

src/main/java/org/miracum/etl/fhirtoomop/repository/VisitDetailRepository.java

+  @Transactional
+  @Modifying
+  @Query(value = "DELETE FROM visit_detail WHERE visit_detail_id NOT IN (select min(visit_detail_id) from cds_cdm.visit_detail where fhir_logical_id = :fhir_logical_id group by visit_detail_id)", nativeQuery = true)


Is there any specific reason to delete rows which does not have given fhirLogicalId ?

This not required can use the method already provided. Removing it.

SatwikSShanbhag · 2024-04-18T14:25:24Z

src/main/java/org/miracum/etl/fhirtoomop/repository/ProviderRepository.java

+        @Transactional
+        @Modifying
+        @Query(value = "DELETE FROM provider WHERE fhir_logical_id = :fhir_logical_id", nativeQuery = true)
+        void deleteProviderByLogicId(@Param("fhir_logical_id") String fhirLogicalId);


same try this void deleteByFhirLogicalId(String fhirLogicalId);

deleteByFhirLogicalId(String fhirLogicalId) method does not exist in the ProviderRepository that is why I have added this to delete the entry by fhirLogicalId.

its available by default just add and test

SatwikSShanbhag

please have a look at comments. even some of comments from previous review not addressed properly.
thanks

SatwikSShanbhag · 2024-04-19T12:26:32Z

src/main/java/org/miracum/etl/fhirtoomop/listeners/OrganizationStepListener.java

+            dbMappings
+                    .getOmopConceptMapWrapper()
+                    .setFindValidSnomedConcept(
+                            repositories.getConceptRepository().findValidConceptId(VOCABULARY_SNOMED));
+            dbMappings.setFindSnomedRaceStandardMapping(
+                    repositories.getSnomedRaceRepository().getSnomedRaceMap());
+            dbMappings
+                    .getOmopConceptMapWrapper()
+                    .setFindValidIPRDConcept(
+                            repositories.getConceptRepository().findValidConceptId(VOCABULARY_IPRD));
+            dbMappings.getOmopConceptMapWrapper().setFindValidWHOConcept(
+                    repositories.getConceptRepository().findValidConceptId(VOCABULARY_WHO)
+            );


is there any reason to move this code inside BulkLoad as I can see even in incremental load we need these setters ?

I am moving these setters outside the block to do this for both bulkload and incremental load.

SatwikSShanbhag · 2024-04-19T12:30:17Z

src/main/java/org/miracum/etl/fhirtoomop/mapper/EncounterDepartmentCaseMapper.java

+  private final OmopRepository repositories;
+


also remove the variable if not used .

SatwikSShanbhag · 2024-04-19T12:32:02Z

src/main/java/org/miracum/etl/fhirtoomop/mapper/EncounterDepartmentCaseMapper.java

+          DbMappings dbMappings,
+          OmopRepository repositories) {


we don't need to pass repository here remove it

SatwikSShanbhag · 2024-04-19T12:34:13Z

src/main/java/org/miracum/etl/fhirtoomop/mapper/EncounterDepartmentCaseMapper.java


    this.bulkload = bulkload;
    this.dbMappings = dbMappings;
+    this.repositories = repositories;


remove this assigning of variable

SatwikSShanbhag · 2024-04-19T12:42:16Z

src/main/java/org/miracum/etl/fhirtoomop/mapper/EncounterDepartmentCaseMapper.java

      String departmentCaseLogicId, String departmentCaseLogicIdentifier) {
    if (!Strings.isNullOrEmpty(departmentCaseLogicId)) {
-      departmentCaseMapperService.deleteExistingDepartmentcaseByLogicalId(departmentCaseLogicId);
+      departmentCaseMapperService.deleteExistingDepartmentCaseByFhirLogicalId(departmentCaseLogicId);


can you use previous function why we required this new function we already have deleteExistingDepartmentcaseByLogicalId which does the same thing.

Its just renamed reverting back to the previous name to avoid any confusion.

SatwikSShanbhag · 2024-04-22T05:21:50Z

src/main/java/org/miracum/etl/fhirtoomop/mapper/ObservationMapper.java


-    var quantityUnitCodingFormat =
-        new Coding().setCode(valueQuantity.getCode()).setSystem(valueQuantity.getSystem());
+    if (valueQuantity.hasSystem()){


how about units ? are they getting stored ?

SatwikSShanbhag · 2024-04-22T05:56:33Z

.../org/miracum/etl/fhirtoomop/repository/service/EncounterDepartmentCaseMapperServiceImpl.java

-  /**
-   * Delete FHIR Encounter resources from OMOP CDM tables using fhir_logical_id
-   *
-   * @param fhirLogicalId logical id of the FHIR Encounter resource
-   */
-  public void deleteExistingDepartmentcaseByLogicalId(String fhirLogicalId) {
-    visitDetailRepository.deleteByFhirLogicalId(fhirLogicalId);
-  }
-


why this has been removed ? the new function which is added below is the same function with different name

Not being removed. I have just changed the function name reverting back to avoid confusion.

SatwikSShanbhag · 2024-04-22T09:03:56Z

src/main/java/org/miracum/etl/fhirtoomop/repository/CareSiteRepository.java

+  @Transactional
+  @Modifying
+  @Query(value = "DELETE FROM care_Site WHERE fhir_logical_id = :fhir_logical_id", nativeQuery = true)
+  void deleteCareSiteByLogicalId(@Param("fhir_logical_id") String fhirLogicalId);


use void deleteByFhirLogicalId(String fhirLogicalId); its available by default

SatwikSShanbhag · 2024-04-22T09:04:28Z

src/main/java/org/miracum/etl/fhirtoomop/repository/ProviderRepository.java

+        @Transactional
+        @Modifying
+        @Query(value = "DELETE FROM provider WHERE fhir_logical_id = :fhir_logical_id", nativeQuery = true)
+        void deleteProviderByLogicId(@Param("fhir_logical_id") String fhirLogicalId);


its available by default just add and test

SatwikSShanbhag · 2024-04-22T09:09:28Z

src/main/java/org/miracum/etl/fhirtoomop/mapper/helpers/ResourceOmopReferenceUtils.java

+//      readerTemplate.update(
+//          String.format("UPDATE %s SET last_updated_at=? WHERE fhir_id=?", inputTableName),
+//          LocalDateTime.now().plusDays(1),
+//          resourceId.substring(4));


according to discussion we had this was commented before only. but here I can see we are commenting it
please explain more on this. pipeline id failing if we keep this piece of code.

AdityaKBhadragond14 · 2024-04-22T10:02:45Z

Adding units to the measurement table can be a different PR as it there needs to be some discussion done on this.
I have commented the lines in the ResourceOmopReferenceUtils.java class because those are to do with some update in the fhirGatway jdbc which we does not require and will fail in our case because we dont use it.

SatwikSShanbhag

Looks good to me thanks

AdityaKBhadragond14 and others added 6 commits March 11, 2024 19:38

Fixed incremental load flow.

241bf9e

Additional Mapper fixes for incremental load flow.

15d0048

Added ON DELETE CASCADE to foreign key constraints.

d0f2555

Merge branch 'main' into ab/incremented_load_flow

135d594

Added missing query in the VisitDetailRepository

f95f208

Added missing query in the VisitDetailRepository

e941c5f

vbothe23 reviewed Apr 15, 2024

View reviewed changes

SatwikSShanbhag requested changes Apr 18, 2024

View reviewed changes

Changes.

172d3e8

AdityaKBhadragond14 requested a review from SatwikSShanbhag April 19, 2024 09:17

Changes.

423cabb

SatwikSShanbhag requested changes Apr 22, 2024

View reviewed changes

Changes.

2fe2565

AdityaKBhadragond14 requested a review from SatwikSShanbhag April 22, 2024 10:06

SatwikSShanbhag approved these changes Apr 23, 2024

View reviewed changes

SatwikSShanbhag requested review from nsabale7 and vbothe23 April 23, 2024 06:08

Conversation

AdityaKBhadragond14 commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SatwikSShanbhag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdityaKBhadragond14 Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SatwikSShanbhag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdityaKBhadragond14 Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdityaKBhadragond14 commented Mar 12, 2024 •

edited

Loading

AdityaKBhadragond14 Apr 19, 2024 •

edited

Loading

AdityaKBhadragond14 Apr 22, 2024 •

edited

Loading