Worked on removing dups on the fact as well as removing the begin and… #686
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… rollback trans on the script
Reviewer's GuideThis PR simplifies the FactDefaulterTracing load script by removing explicit transaction boundaries, adding CTEs with row_number-based logic to deduplicate both differentiated care and defaulter tracing records, and updating the main visits_data CTE to consume these deduplicated sources and adjust field selections. Entity relationship diagram for deduplication logic in FactDefaulterTracing loaderDiagram
ODS_Care_CT_PatientVisits {
int SiteCode
int PatientPK
string PatientPKHash
string DifferentiatedCare
date VisitDate
int VisitID
}
ODS_Intermediate_Intermediate_LastVisitDate {
int SiteCode
int PatientPK
date LastVisitDate
}
ODS_Care_CT_DefaulterTracing {
int SiteCode
int PatientPK
string PatientPKHash
string PatientIDHash
date VisitDate
int VisitID
string Comments
string TracingType
string TracingOutcome
string IsFinalTrace
}
ODS_Care_CT_Patient {
string PatientPKHash
int SiteCode
date DOB
}
Latest_Differentiated_Rank {
int SiteCode
int PatientPK
string PatientPKHash
string DifferentiatedCare
date VisitDate
int VisitID
}
Latest_Latest_DefaulterTracing_Ordered_Rank {
int SiteCode
int PatientPK
string PatientPKHash
string PatientIDHash
date VisitDate
int VisitID
string Comments
string TracingType
string TracingOutcome
string IsFinalTrace
}
visits_data {
string PatientPKHash
string PatientIDHash
int SiteCode
int VisitID
date VisitDate
string TracingType
string TracingOutcome
string IsFinalTrace
string Comments
string DifferentiatedCare
int AgeAtVisit
}
ODS_Care_CT_PatientVisits ||--o{ ODS_Intermediate_Intermediate_LastVisitDate : "joins on SiteCode, PatientPK, VisitDate"
ODS_Care_CT_PatientVisits ||--o{ Latest_Differentiated_Rank : "deduped by PatientPK, SiteCode, VisitID"
ODS_Care_CT_DefaulterTracing ||--o{ Latest_Latest_DefaulterTracing_Ordered_Rank : "deduped by PatientPK, SiteCode, VisitID"
Latest_Latest_DefaulterTracing_Ordered_Rank ||--o{ visits_data : "source for defaulter tracing"
Latest_Differentiated_Rank ||--o{ visits_data : "source for differentiated care"
ODS_Care_CT_Patient ||--o{ visits_data : "join on PatientPKHash, SiteCode"
Class diagram for new deduplication CTEs in FactDefaulterTracing loadclassDiagram
class Latest_Differentiated_Ordered {
+int num
+int SiteCode
+date VisitDate
+int VisitID
+string PatientPKHash
+string DifferentiatedCare
}
class Latest_Differentiated_Rank {
+int SiteCode
+date VisitDate
+int VisitID
+string PatientPKHash
+string DifferentiatedCare
}
class Latest_DefaulterTracing_Ordered {
+int num
+int SiteCode
+int PatientPK
+string PatientPKHash
+string PatientIDHash
+date VisitDate
+int VisitID
+string Comments
+string TracingType
+string TracingOutcome
+string IsFinalTrace
}
class Latest_Latest_DefaulterTracing_Ordered_Rank {
+int SiteCode
+int PatientPK
+string PatientPKHash
+string PatientIDHash
+date VisitDate
+int VisitID
+string Comments
+string TracingType
+string TracingOutcome
+string IsFinalTrace
}
Latest_Differentiated_Ordered <|-- Latest_Differentiated_Rank : filter num=1
Latest_DefaulterTracing_Ordered <|-- Latest_Latest_DefaulterTracing_Ordered_Rank : filter num=1
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @DennisGibz - I've reviewed your changes - here's some feedback:
- Remove the unnecessary
distinct *in the ranking CTEs (row_number already deduplicates) and explicitly list columns to guard against future schema changes. - Since the
begin tran/rollback tranwere removed, consider wrapping the final insert in a transaction with TRY/CATCH so failures won’t leave the table in a partial state. - Simplify and standardize the verbose CTE names (e.g., rename
Latest_Latest_DefaulterTracing_Ordered_Rankto something shorter) for better readability and maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Remove the unnecessary `distinct *` in the ranking CTEs (row_number already deduplicates) and explicitly list columns to guard against future schema changes.
- Since the `begin tran`/`rollback tran` were removed, consider wrapping the final insert in a transaction with TRY/CATCH so failures won’t leave the table in a partial state.
- Simplify and standardize the verbose CTE names (e.g., rename `Latest_Latest_DefaulterTracing_Ordered_Rank` to something shorter) for better readability and maintainability.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
nobert-mumo
reviewed
Aug 11, 2025
| DROP TABLE [NDWH].[Fact].[FactDefaulterTracing]; | ||
|
|
||
| with MFL_partner_agency_combination as ( | ||
| ;with MFL_partner_agency_combination as ( |
There was a problem hiding this comment.
probably misplaced semi colon ?
There was a problem hiding this comment.
@nobert-mumo the semicolon just works fine.
| ,visits.VisitDate | ||
| ,visits.visitID | ||
| from ODS.Care.CT_PatientVisits as visits | ||
| inner join ODS.[Intermediate].Intermediate_LastVisitDate as last_visit on visits.SiteCode = last_visit.SiteCode |
There was a problem hiding this comment.
@DennisGibz BTW what does the VisitID imply for this intermediate? Should we do the partitioning instead on this table so that it is consistent for all other Facts ?
There was a problem hiding this comment.
@nobert-mumo I didn't get above.
There was a problem hiding this comment.
@DennisGibz I meant, my assumption is the addition of the visitID here is to make the rows unique, if that is the case, does it mean all other Facts that utilise Intermediate_LastVisitDate could also suffer the same issues as this fact ?
There was a problem hiding this comment.
Intermediate_LastVisitDate we don't use visitID on the partition. Actually I don't think we have any table that we use visitID as the unique Identifier
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
… rollback trans on the script
Summary by Sourcery
Refine the load_FactDefaulterTracing.sql script by removing manual transaction control and adding CTE-based deduplication for differentiated care and defaulter tracing data before populating the fact table.
Enhancements: