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

Bug Fix: Return correct count values for RC/DRC and PC/DPC #2405

Merged
merged 8 commits into from
Nov 8, 2024

Conversation

amarsan
Copy link
Collaborator

@amarsan amarsan commented Nov 7, 2024

Closes #2296

The issue is that we were modifying an array of concept IDs when going to the DB for their counts and caching them. I wrote an integration test cover this scenario.

I had some trouble getting the integration tests to run in my environment, and ended up having to add a dependency to the pom.xml file.

<artifactId>asm</artifactId>
<version>1.0.2</version>
<scope>test</scope>
</dependency>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I needed to add this dependency in order to get the project to build and run in my test runner.

Copy link
Collaborator

Choose a reason for hiding this comment

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

should be fine, the scope is test so won't be part of the final bundle.

List<CDMCacheEntity> zeroConcepts = idsSlice.stream().map(id -> {
List<Integer> notFoundIds = new ArrayList<Integer>(CollectionUtils.subtract(idsSlice, cachedIds));
if (!notFoundIds.isEmpty()) { // store zeros in cache
List<CDMCacheEntity> zeroConcepts = notFoundIds.stream().map(id -> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the bug fix (all the other files are for adding the integration test). The idsSlice variable is a subList of the ids that were passed into this method. In Java, an array subList references the parent list that it came from, and thus any structural changes to it affect the parent list. We were removing the cachedIds from slice, which also removed them from the parent ids list. After this method is called, the calling method uses the ids list to determine what to return the front end, so what was happening was that any counts we had to get from the DB were not being returned.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Very good explanation. Thank you for researching this and providing a patch.

@chrisknoll chrisknoll merged commit bbdea77 into OHDSI:master Nov 8, 2024
2 checks passed
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.

Atlas 2.13.0 returns wrong values for RC/DRC and PC/DPC
2 participants