From 28c9d0d7c95837fae424cb9856574f732810e1fb Mon Sep 17 00:00:00 2001 From: Test Date: Sun, 4 Oct 2026 03:22:15 +0000 Subject: [PATCH 1/2] fix(ENGKNOW-3978): refbases on contig without sequence returns N RefSeqFromChromSeq.getBases read lastBuff after warming via getBase, but getBase never updated lastKey/lastBuff when the contig had no chromSeq file. Result: bases from the previous contig's buffer, or an NPE when no buffer had been loaded yet. Only use lastBuff when lastKey matches the requested buffer, otherwise fall back to per-base getBase which returns 'N'. getBase now sets lastKey/lastBuff on a fresh read, removing the double-call warm-up. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../gor/iterators/RefSeqFromChromSeq.scala | 24 +++++++++++-------- .../iterators/UTestRefSeqFromChromSeq.java | 24 +++++++++++++++++++ 2 files changed, 38 insertions(+), 10 deletions(-) diff --git a/model/src/main/scala/org/gorpipe/model/gor/iterators/RefSeqFromChromSeq.scala b/model/src/main/scala/org/gorpipe/model/gor/iterators/RefSeqFromChromSeq.scala index d17c78b57..4164950be 100644 --- a/model/src/main/scala/org/gorpipe/model/gor/iterators/RefSeqFromChromSeq.scala +++ b/model/src/main/scala/org/gorpipe/model/gor/iterators/RefSeqFromChromSeq.scala @@ -147,6 +147,8 @@ class RefSeqFromChromSeq(ipath : String, fileReader : FileReader) extends RefSeq f.get().seek(offset) val l = f.get().read(buff, 0, buffLength) lufo.addObject(buffKey, buff) + lastKey = buffKey + lastBuff = buff if( l == -1 ) { log.warn("Trying to read "+chr+":"+pos+" from reference file " + chrFilePath + " of length "+f.get.length()+" from offset " + offset) return 'N' @@ -169,17 +171,19 @@ class RefSeqFromChromSeq(ipath : String, fileReader : FileReader) extends RefSeq if ((pos1 - 1) / buffLength == (pos2 - 1) / buffLength) { val (buffKey, offset) = getKeyAndOffset(chr, pos1) - if (buffKey != lastKey) { - val temp = getBase(chr, pos1) - val temp2 = getBase(chr, pos1) + if (buffKey != lastKey) getBase(chr, pos1) + + // lastBuff is only valid for lastKey. If the buffer could not be loaded (e.g. the contig has no + // sequence in the build) fall through to getBase per position, which returns 'N'. + if (buffKey == lastKey) { + val strbuff = new StringBuilder(pos2 - pos1 + 1) + var i = pos1 + while (i <= pos2) { + strbuff.append(refByteToChar(lastBuff(i - offset - 1))) + i += 1 + } + return strbuff.toString } - val strbuff = new StringBuilder(pos2 - pos1 + 1) - var i = pos1 - while (i <= pos2) { - strbuff.append(refByteToChar(lastBuff(i - offset - 1))) - i += 1 - } - return strbuff.toString } val strbuff = new StringBuilder(pos2 - pos1 + 1) var i = pos1 diff --git a/model/src/test/java/org/gorpipe/model/gor/iterators/UTestRefSeqFromChromSeq.java b/model/src/test/java/org/gorpipe/model/gor/iterators/UTestRefSeqFromChromSeq.java index 8f0948458..a3ae44b13 100644 --- a/model/src/test/java/org/gorpipe/model/gor/iterators/UTestRefSeqFromChromSeq.java +++ b/model/src/test/java/org/gorpipe/model/gor/iterators/UTestRefSeqFromChromSeq.java @@ -79,6 +79,30 @@ public void testGetRefbases() { } + @Test + public void testGetRefbasesMissingContigAfterOtherContig() { + RefSeqFromChromSeq refseq = new RefSeqFromChromSeq("../tests/data/ref_mini/chromSeq", new DriverBackedFileReader("")); + + // Warm the buffer with a contig that has a sequence. + Assert.assertEquals("CAG", refseq.getBases("chr1", 101000, 101002)); + + // Contig without a sequence file must not return bases from the previous contig's buffer. + Assert.assertEquals("NNN", refseq.getBases("chrXY", 101000, 101002)); + Assert.assertEquals('N', refseq.getBase("chrXY", 101000)); + + // Switching back still works. + Assert.assertEquals("CAG", refseq.getBases("chr1", 101000, 101002)); + } + + @Test + public void testGetRefbasesMissingContigFirst() { + RefSeqFromChromSeq refseq = new RefSeqFromChromSeq("../tests/data/ref_mini/chromSeq", new DriverBackedFileReader("")); + + // First read on a contig without a sequence file must not fail. + Assert.assertEquals("NN", refseq.getBases("chrXY", 10, 11)); + Assert.assertEquals("CAG", refseq.getBases("chr1", 101000, 101002)); + } + @Ignore("Run manually to test from same buffer optimization") @Test public void testGetRefbasesPerformance() { From a01670388c6e2b084969279132f263651c20f523 Mon Sep 17 00:00:00 2001 From: Test Date: Tue, 6 Oct 2026 22:13:20 +0000 Subject: [PATCH 2/2] fix(ENGKNOW-3978): load each refseq buffer once per getBases call Move buffer loading into getBuffer, which returns null for a contig without sequence or a failed read and only sets lastKey/lastBuff on success. getBases now walks the range buffer by buffer and fills a buffer that can not be loaded with 'N' in one step. Before, a failed read (e.g. a wrapped network error) was retried and logged once per position, and a missing contig rebuilt the file path per position. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../gor/iterators/RefSeqFromChromSeq.scala | 145 +++++++++--------- .../iterators/UTestRefSeqFromChromSeq.java | 36 +++++ 2 files changed, 111 insertions(+), 70 deletions(-) diff --git a/model/src/main/scala/org/gorpipe/model/gor/iterators/RefSeqFromChromSeq.scala b/model/src/main/scala/org/gorpipe/model/gor/iterators/RefSeqFromChromSeq.scala index 1297067f8..b1de43c6e 100644 --- a/model/src/main/scala/org/gorpipe/model/gor/iterators/RefSeqFromChromSeq.scala +++ b/model/src/main/scala/org/gorpipe/model/gor/iterators/RefSeqFromChromSeq.scala @@ -113,59 +113,11 @@ class RefSeqFromChromSeq(ipath : String, fileReader : FileReader) extends RefSeq } def getBase(chr: String, pos: Int): Char = { - if (noReferenceBuildFound) return 'N' // Positions below 1 are outside the chromosome, same as positions past its end. if (pos < 1) return 'N' - try { - val (buffKey, offset) = getKeyAndOffset(chr, pos) - - if (buffKey == lastKey) return refByteToChar(lastBuff(pos - offset - 1)) - lufo.getObject(buffKey) match { - case Some(buffer) => - lastKey = buffKey - lastBuff = buffer - refByteToChar(buffer(pos - offset - 1)) - case None => - val chrFilePath = DataUtil.toFile(path + "/" + chr, DataType.TXT) - val f = if( filemap.containsKey(chrFilePath) ) filemap.get(chrFilePath) else { - val cf = Optional.ofNullable(fileReader match { - case dbfr: DriverBackedFileReader => - val ds = dbfr.unsecure().resolveUrl(chrFilePath) - if (ds.exists()) new StreamSourceRacFile(ds.asInstanceOf[StreamSource]) else null - case _ => - fileReader.openFile(chrFilePath) - }) - filemap.put(chrFilePath, cf) - cf - } - if( !f.isPresent ) { - if (!notfoundmap.contains(chrFilePath)) { - notfoundmap.add(chrFilePath) - log.warn("Reference build " + path + "\n\nReference file "+chrFilePath+" does not exist", chrFilePath) - } - 'N' - } else { - val buff = new Array[Byte](buffLength) - f.get().seek(offset) - val l = f.get().read(buff, 0, buffLength) - lufo.addObject(buffKey, buff) - lastKey = buffKey - lastBuff = buff - if( l == -1 ) { - log.warn("Trying to read "+chr+":"+pos+" from reference file " + chrFilePath + " of length "+f.get.length()+" from offset " + offset) - return 'N' - } - refByteToChar(buff(pos - offset - 1)) - } - } - } catch { - case ioex: IOException => - throw new GorResourceException("Reference build " + path + " inaccessible", path, ioex) - case ex: Exception => { - log.warn(String.format("Returning 'N' for reference build %s (%s:%d)", path, chr, pos), ex) - } - 'N' - } + val (buffKey, offset) = getKeyAndOffset(chr, pos) + val buff = getBuffer(chr, pos, buffKey, offset) + if (buff == null) 'N' else refByteToChar(buff(pos - offset - 1)) } def getBases(chr: String, pos1: Int, pos2: Int): String = { @@ -176,32 +128,85 @@ class RefSeqFromChromSeq(ipath : String, fileReader : FileReader) extends RefSeq return if (pos2 < 1) leading else leading + getBases(chr, 1, pos2) } if (pos1 == pos2) return getBase(chr, pos1).toString - if ((pos1 - 1) / buffLength == (pos2 - 1) / buffLength) { - val (buffKey, offset) = getKeyAndOffset(chr, pos1) - - if (buffKey != lastKey) getBase(chr, pos1) - - // lastBuff is only valid for lastKey. If the buffer could not be loaded (e.g. the contig has no - // sequence in the build) fall through to getBase per position, which returns 'N'. - if (buffKey == lastKey) { - val strbuff = new StringBuilder(pos2 - pos1 + 1) - var i = pos1 - while (i <= pos2) { - strbuff.append(refByteToChar(lastBuff(i - offset - 1))) + val strbuff = new StringBuilder(pos2 - pos1 + 1) + var start = pos1 + // Load each buffer the range touches once. A buffer that can not be loaded reads as 'N' for its whole part. + while (start <= pos2) { + val (buffKey, offset) = getKeyAndOffset(chr, start) + val end = math.min(pos2, offset + buffLength) + val buff = getBuffer(chr, start, buffKey, offset) + if (buff == null) { + strbuff.append("N" * (end - start + 1)) + } else { + var i = start + while (i <= end) { + strbuff.append(refByteToChar(buff(i - offset - 1))) i += 1 } - return strbuff.toString } - } - val strbuff = new StringBuilder(pos2 - pos1 + 1) - var i = pos1 - while (i <= pos2) { - strbuff.append(getBase(chr, i)) - i += 1 + start = end + 1 } strbuff.toString } + /** + * Get the buffer for buffKey, from lastBuff, the LUFO cache or the reference file. + * lastKey/lastBuff are only set when a buffer is returned, so lastBuff always belongs to lastKey. + * @return the buffer, or null if the contig has no sequence in the build or the read failed. + */ + private def getBuffer(chr: String, pos: Int, buffKey: String, offset: Int): Array[Byte] = { + if (noReferenceBuildFound) return null + if (buffKey == lastKey) return lastBuff + try { + val buff = lufo.getObject(buffKey) match { + case Some(buffer) => buffer + case None => readBuffer(chr, pos, buffKey, offset) + } + if (buff != null) { + lastKey = buffKey + lastBuff = buff + } + buff + } catch { + case ioex: IOException => + throw new GorResourceException("Reference build " + path + " inaccessible", path, ioex) + case ex: Exception => + log.warn(String.format("Returning 'N' for reference build %s (%s:%d)", path, chr, pos), ex) + null + } + } + + private def readBuffer(chr: String, pos: Int, buffKey: String, offset: Int): Array[Byte] = { + val chrFilePath = DataUtil.toFile(path + "/" + chr, DataType.TXT) + val f = if( filemap.containsKey(chrFilePath) ) filemap.get(chrFilePath) else { + val cf = Optional.ofNullable(fileReader match { + case dbfr: DriverBackedFileReader => + val ds = dbfr.unsecure().resolveUrl(chrFilePath) + if (ds.exists()) new StreamSourceRacFile(ds.asInstanceOf[StreamSource]) else null + case _ => + fileReader.openFile(chrFilePath) + }) + filemap.put(chrFilePath, cf) + cf + } + if( !f.isPresent ) { + if (!notfoundmap.contains(chrFilePath)) { + notfoundmap.add(chrFilePath) + log.warn("Reference build " + path + "\n\nReference file "+chrFilePath+" does not exist", chrFilePath) + } + return null + } + val buff = new Array[Byte](buffLength) + f.get().seek(offset) + val l = f.get().read(buff, 0, buffLength) + lufo.addObject(buffKey, buff) + if( l == -1 ) { + // Past the end of the chromosome, the buffer stays zero and reads as 'N'. + log.warn("Trying to read "+chr+":"+pos+" from reference file " + chrFilePath + " of length "+f.get.length()+" from offset " + offset) + } + buff + } + /** * Convert reference byte to reference char. * @param b byte to convert. diff --git a/model/src/test/java/org/gorpipe/model/gor/iterators/UTestRefSeqFromChromSeq.java b/model/src/test/java/org/gorpipe/model/gor/iterators/UTestRefSeqFromChromSeq.java index 928d8eeb0..d7e0b321a 100644 --- a/model/src/test/java/org/gorpipe/model/gor/iterators/UTestRefSeqFromChromSeq.java +++ b/model/src/test/java/org/gorpipe/model/gor/iterators/UTestRefSeqFromChromSeq.java @@ -1,6 +1,8 @@ package org.gorpipe.model.gor.iterators; import org.gorpipe.gor.model.DriverBackedFileReader; +import org.gorpipe.gor.model.FileReader; +import org.gorpipe.gor.model.RacFile; import org.junit.Assert; import org.junit.Ignore; import org.junit.Rule; @@ -9,7 +11,9 @@ import org.junit.contrib.java.lang.system.RestoreSystemProperties; import org.junit.contrib.java.lang.system.SystemErrRule; import org.junit.rules.TemporaryFolder; +import org.mockito.Mockito; +import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; @@ -103,6 +107,38 @@ public void testGetRefbasesMissingContigFirst() { Assert.assertEquals("CAG", refseq.getBases("chr1", 101000, 101002)); } + @Test + public void testGetRefbasesMissingContigAcrossBuffers() { + RefSeqFromChromSeq refseq = new RefSeqFromChromSeq("../tests/data/ref_mini/chromSeq", new DriverBackedFileReader("")); + + Assert.assertEquals("CAG", refseq.getBases("chr1", 101000, 101002)); + Assert.assertEquals("N".repeat(10006), refseq.getBases("chrXY", 9998, 20003)); + } + + @Test + public void testGetRefbasesFailedReadIsTriedOncePerBuffer() throws IOException { + // A mocked FileReader has no absolute paths for the cache folder lookup. + System.clearProperty("gor.refseq.cache.folder"); + + RacFile failingFile = Mockito.mock(RacFile.class); + Mockito.when(failingFile.read(Mockito.any(byte[].class), Mockito.anyInt(), Mockito.anyInt())) + .thenThrow(new RuntimeException("simulated read failure")); + FileReader fileReader = Mockito.mock(FileReader.class); + Mockito.when(fileReader.openFile(Mockito.anyString())).thenReturn(failingFile); + + RefSeqFromChromSeq refseq = new RefSeqFromChromSeq("refbuild", fileReader); + + // Range within one buffer: one read attempt, not one per position. + Assert.assertEquals("NNN", refseq.getBases("chr1", 101000, 101002)); + Mockito.verify(failingFile, Mockito.times(1)).read(Mockito.any(byte[].class), Mockito.anyInt(), Mockito.anyInt()); + + // Range across a buffer boundary: one read attempt per buffer. + Assert.assertEquals("NNN", refseq.getBases("chr1", 109999, 110001)); + Mockito.verify(failingFile, Mockito.times(3)).read(Mockito.any(byte[].class), Mockito.anyInt(), Mockito.anyInt()); + + Assert.assertEquals('N', refseq.getBase("chr1", 101000)); + } + // Positions below 1 (e.g. pos 0 from an unmapped liftover row) read as 'N', like positions past the chromosome end. @Test public void testGetRefbasesBelowPositionOne() {