-
Notifications
You must be signed in to change notification settings - Fork 354
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
feat: Merge Category Option Combo [DHIS2-18321] #19488
base: master
Are you sure you want to change the base?
Conversation
dhis-2/dhis-api/src/main/java/org/hisp/dhis/dataapproval/DataApprovalStore.java
Fixed
Show fixed
Hide fixed
dhis-2/dhis-api/src/main/java/org/hisp/dhis/datavalue/DataValue.java
Outdated
Show resolved
Hide resolved
* @param uids {@link CategoryOptionCombo} {@link UID}s | ||
* @return {@link Event}s with references to {@link CategoryOptionCombo} {@link UID} passed in | ||
*/ | ||
List<Event> getAllByAttributeOptionCombo(Collection<UID> uids); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking, have we considered the performance and memory use here? Is it possible that the arguments will include the default option combo, and then get millions of events returned, leading to OOM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Do you think we should reject merge requests if they include the default cat option combo? It probably doesn't make sense if the default is part of a merge does it? I know some systems have multiple defaults, so we would only consider our own default that we set.
I will also post this question in the Jira issue for others too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understand. I think for the org unit merge we use HQL/SQL for the merge? I think for large databases with 10m+ events there may be no other choice.
...es/dhis-service-administration/src/main/java/org/hisp/dhis/merge/CommonDataMergeHandler.java
Outdated
Show resolved
Hide resolved
Quality Gate passedIssues Measures |
Feature
Adds a new feature, which allows the merging of
CategoryOptionCombo
s.Extracted the handling of merging
DataValue
s into a common handler class as this is used by multiple merge types (DataElement
&CategoryOptionCombo
).Testing
Automated
Tests added for
Docs
PR - dhis2/dhis2-docs#1466