EVA-3543 check if rs with same hash already exist before merging #441
Conversation
| existingClusteredVariantEntity.getAccession()).accessionToKeep) { | ||
| merge(mergeDestination, existingClusteredVariantEntity, currentOperation); |
There was a problem hiding this comment.
The existingClusteredVariantEntity is also part of the mergees so we will likely have to remove it from the list
| @@ -192,6 +193,18 @@ public void writeRSMerge(SubmittedVariantOperationEntity currentOperation) | |||
| getMergeDestinationAndMergees(mergeCandidates); | |||
| ClusteredVariantEntity mergeDestination = mergeDestinationAndMergees.getLeft(); | |||
|
|
|||
| // check if any variant with same hash as mergeDestination already exist in DB | |||
There was a problem hiding this comment.
This section is only relevant if the list of mergees has more than 2 elements. Can we test this before going through this ?
There was a problem hiding this comment.
Does it also work (and maybe make more sense) to actually just extract this removal into a method that's called before the for-loop, and modify it so it checks for all mergee accessions instead of just one?
I'm sure this implementation works, but I'm wary of essentially redo-ing merge detection and prioritisation logic rather than just trusting the candidates we have.
| } else { | ||
| merge(existingClusteredVariantEntity, mergeDestination, currentOperation); | ||
| mergeDestination = existingClusteredVariantEntity; |
There was a problem hiding this comment.
I don't think this part is necessary. If the existing cluster variant entity document is the merge destination then the norm code bellow would work.
...sion-clustering/src/main/java/uk/ac/ebi/eva/accession/clustering/batch/io/RSMergeWriter.java
Outdated
Show resolved
Hide resolved
...sion-clustering/src/main/java/uk/ac/ebi/eva/accession/clustering/batch/io/RSMergeWriter.java
Show resolved
Hide resolved
No description provided.