You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@uima.apache.org by re...@apache.org on 2020/11/10 13:25:53 UTC
[uima-uimaj] 01/01: [UIMA-6296] SelectFS fsIterator may yield
different result set than asList
This is an automated email from the ASF dual-hosted git repository.
rec pushed a commit to branch bugfix/UIMA-6296-SelectFS-fsIterator-may-yield-different-result-set-than-asList
in repository https://gitbox.apache.org/repos/asf/uima-uimaj.git
commit 5148df8075600779f78f57d92d19b9b7f22b876e
Author: Richard Eckart de Castilho <re...@apache.org>
AuthorDate: Tue Nov 10 14:25:50 2020 +0100
[UIMA-6296] SelectFS fsIterator may yield different result set than asList
- Add test of consitency of forward-iteration through select results with predicates
---
.../apache/uima/cas/impl/FsIterator_limited.java | 26 ++-
.../org/apache/uima/cas/impl/SelectFSs_impl.java | 188 +++++++++++++++------
.../org/apache/uima/cas/impl/SelectFsAssert.java | 125 +++++++-------
.../cas/impl/SelectFsPredicateAlignmentTest.java | 1 -
.../org/apache/uima/cas/impl/SelectFsTest.java | 1 -
.../uima/cas/test/AnnotationIteratorTest.java | 20 +--
6 files changed, 228 insertions(+), 133 deletions(-)
diff --git a/uimaj-core/src/main/java/org/apache/uima/cas/impl/FsIterator_limited.java b/uimaj-core/src/main/java/org/apache/uima/cas/impl/FsIterator_limited.java
index bcd0990..ac9f1d0 100644
--- a/uimaj-core/src/main/java/org/apache/uima/cas/impl/FsIterator_limited.java
+++ b/uimaj-core/src/main/java/org/apache/uima/cas/impl/FsIterator_limited.java
@@ -19,15 +19,16 @@
package org.apache.uima.cas.impl;
-import java.util.Arrays;
import java.util.Comparator;
+import java.util.NoSuchElementException;
import org.apache.uima.cas.FSIterator;
import org.apache.uima.cas.FeatureStructure;
import org.apache.uima.jcas.cas.TOP;
/**
- * Wraps FSIterator<T>, limits results to n gets
+ * Wraps FSIterator<T>, limits results to n gets. Moving the iterator around does not count towards
+ * the limit.
*/
class FsIterator_limited<T extends FeatureStructure>
implements LowLevelIterator<T> {
@@ -35,22 +36,26 @@ class FsIterator_limited<T extends FeatureStructure>
final private LowLevelIterator<T> iterator; // not just for single-type iterators
final private int limit;
private int count = 0;
+ private boolean limitReached = false;
FsIterator_limited(FSIterator<T> iterator, int limit) {
this.iterator = (LowLevelIterator<T>) iterator;
this.limit = limit;
+ this.limitReached = limit <= count;
}
private void maybeMakeInvalid() {
if (count == limit) {
- iterator.moveToFirstNoReinit();
- iterator.moveToPrevious();
+ limitReached = true;
}
}
@Override
public T getNvc() {
maybeMakeInvalid();
+ if (limitReached) {
+ throw new NoSuchElementException();
+ }
T r = iterator.get(); // not getNvc because of above line
count++;
return r;
@@ -59,12 +64,18 @@ class FsIterator_limited<T extends FeatureStructure>
@Override
public void moveToNextNvc() {
maybeMakeInvalid();
+ if (limitReached) {
+ return;
+ }
iterator.moveToNext(); // not getNvc because of above line
}
@Override
public void moveToPreviousNvc() {
maybeMakeInvalid();
+ if (limitReached) {
+ return;
+ }
iterator.moveToPrevious(); // not getNvc because of above line
}
@@ -95,13 +106,16 @@ class FsIterator_limited<T extends FeatureStructure>
@Override
public FSIterator<T> copy() {
- return new FsIterator_limited<>(iterator.copy(), limit);
+ FsIterator_limited<T> copy = new FsIterator_limited<>(iterator.copy(), limit);
+ copy.count = count;
+ copy.limitReached = limitReached;
+ return copy;
}
@Override
public boolean isValid() {
maybeMakeInvalid();
- return iterator.isValid();
+ return !limitReached && iterator.isValid();
}
@Override
diff --git a/uimaj-core/src/main/java/org/apache/uima/cas/impl/SelectFSs_impl.java b/uimaj-core/src/main/java/org/apache/uima/cas/impl/SelectFSs_impl.java
index e9710a3..530c6d8 100644
--- a/uimaj-core/src/main/java/org/apache/uima/cas/impl/SelectFSs_impl.java
+++ b/uimaj-core/src/main/java/org/apache/uima/cas/impl/SelectFSs_impl.java
@@ -572,8 +572,6 @@ public class SelectFSs_impl <T extends FeatureStructure> implements SelectFSs<T>
isFollowing || isPreceding;
isUnordered = ! orderingNeeded;
-
-
}
private void maybeValidateAltSource() {
@@ -617,83 +615,60 @@ public class SelectFSs_impl <T extends FeatureStructure> implements SelectFSs<T>
if (isFollowing && isBackwards) {
isBackwards = false;
return make_or_copy_snapshot(fsIterator1(), true);
-// LowLevelIterator<T> baseIterator = fsIterator1();
-// FSIterator<T> it;
-// if (baseIterator instanceof FsIterator_subtypes_snapshot) {
-// it = new FsIterator_backwards<>(baseIterator.copy()); // avoid making another array
-// } else {
-// T[] a = (T[]) asArray(baseIterator);
-// it = new FsIterator_backwards<>(
-// new FsIterator_subtypes_snapshot<T>(
-// a,
-// (LowLevelIndex<T>) index,
-// IS_ORDERED,
-// baseIterator.getComparator()));
-// }
-// return (limit == -1)
-// ? it
-// // rewrap with limit - needs to be outer shell to get right invalid behavior
-// : new FsIterator_limited<>(it, limit);
}
-
+
if (isPreceding) {
- boolean bkwd = isBackwards; // save isBackwards flag.
-
- isBackwards = true; // because need the iterator to move from the position to the front.
- return make_or_copy_snapshot(fsIterator1(), bkwd); // this iterator fails to skip annotations whose end is > positioning begin
-// LowLevelIterator<T> baseIterator = fsIterator1(); // this iterator fails to skip annotations whose end is > positioning begin
-// T[] a = (T[]) asArray(baseIterator);
-// FSIterator<T> it = new FsIterator_subtypes_snapshot<T>(
-// a,
-// (LowLevelIndex<T>) index,
-// IS_ORDERED,
-// baseIterator.getComparator());
-// if (!bkwd) {
-// it = new FsIterator_backwards<>(it); // because array is backwards
-// }
-// return (limit == -1)
-// ? it
-// // rewrap with limit - needs to be outer shell to get right invalid behavior
-// : new FsIterator_limited<>(it, limit);
+ // save isBackwards flag.
+ boolean bkwd = isBackwards;
+ // because need the iterator to move from the position to the front.
+ isBackwards = true;
+ // this iterator fails to skip annotations whose end is > positioning begin
+ return make_or_copy_snapshot(fsIterator1(), bkwd);
}
-
+
// all others, including isFollowing but not backwards
return fsIterator1();
}
- private FSIterator<T> make_or_copy_snapshot(LowLevelIterator<T> baseIterator, boolean bkwd) {
- FSIterator<T> it;
- T[] a = (T[]) asArray(baseIterator, FeatureStructure.class); // array is in forward order because
- // it's produced by a backwards iterator, but then the array is reversed
- it = new FsIterator_subtypes_snapshot<>(
- a,
+ private LowLevelIterator<T> make_or_copy_snapshot(LowLevelIterator<T> baseIterator, boolean bkwd) {
+ LowLevelIterator<T> it;
+ // array is in forward order because it's produced by a backwards iterator, but then the array
+ // is reversed
+ T[] a = (T[]) asArray(baseIterator, FeatureStructure.class);
+
+ it = new FsIterator_subtypes_snapshot<>(a,
(LowLevelIndex<T>) index,
IS_ORDERED,
baseIterator.getComparator());
-
+
if (!bkwd) {
it = new FsIterator_backwards<>(it);
}
return (limit == -1)
? it
- // rewrap with limit - needs to be outer shell to get right invalid behavior
+ // rewrap with limit - needs to be outer shell to get right invalid behavior
: new FsIterator_limited<>(it, limit);
}
private LowLevelIterator<T> fsIterator1() {
prepareTerminalOp();
- LowLevelIterator<T> it = isAllViews
- ? //new FsIterator_aggregation_common<T>(getPlainIteratorsForAllViews(), )
- createFsIterator_for_all_views()
- : plainFsIterator(index, view);
+ LowLevelIterator<T> it = new SelectFSIterator(() -> {
+ LowLevelIterator<T> baseIt = isAllViews
+ ? createFsIterator_for_all_views()
+ : plainFsIterator(index, view);
+
+ baseIt = maybeWrapBackwards(baseIt);
+ // position needs to come after backwards because that sets the position
+ maybePosition(baseIt);
+ // shift semantically needs to come after backwards
+ maybeShift(baseIt);
+ return baseIt;
+ });
- it = maybeWrapBackwards(it);
- maybePosition(it); // position needs to come after backwards because that sets the position
- maybeShift(it); // shift semantically needs to come after backwards
- return (limit == -1) ? it : new FsIterator_limited<>(it, limit);
+ return (limit == -1) ? it : new FsIterator_limited<>(it, limit);
}
-
+
/**
* for a selected index, return an iterator over all the views for that index
* @return iterator over all views for that index, unordered
@@ -1648,4 +1623,105 @@ public class SelectFSs_impl <T extends FeatureStructure> implements SelectFSs<T>
if (this.limit == 0) return true;
return fsIterator().size() == 0;
}
+
+ public final class SelectFSIterator implements LowLevelIterator<T> {
+
+ private Supplier<LowLevelIterator<T>> iteratorSupplier;
+ private LowLevelIterator<T> it;
+
+ private SelectFSIterator(Supplier<LowLevelIterator<T>> aIteratorSupplier) {
+ iteratorSupplier = aIteratorSupplier;
+
+ it = iteratorSupplier.get();
+ }
+
+ @Override
+ public boolean isValid() {
+ return it.isValid();
+ }
+
+ @Override
+ public T getNvc() {
+ return it.getNvc();
+ }
+
+ @Override
+ public void moveToNextNvc() {
+ it.moveToNextNvc();
+ }
+
+ @Override
+ public void moveToPreviousNvc() {
+ it.moveToPreviousNvc();
+ }
+
+ @Override
+ public void moveToFirst() {
+ // There is quite a bit of logic in SelectFS that happens *after* the iterator has been
+ // created using fsIterator1() that positions the iterator at the starting position required
+ // by the SelectFS settings. In order to avoid having to duplicate that repositioning code,
+ // we just make a new iterator here.
+ it = iteratorSupplier.get();
+ }
+
+ @Override
+ public void moveToLast() {
+ it.moveToLast();
+ }
+
+ @Override
+ public void moveTo(FeatureStructure aFs) {
+ it.moveTo(aFs);
+ }
+
+ @Override
+ public FSIterator<T> copy() {
+ return it.copy();
+ }
+
+ @Override
+ public int ll_indexSizeMaybeNotCurrent() {
+ return it.ll_indexSizeMaybeNotCurrent();
+ }
+
+ @Override
+ public LowLevelIndex<T> ll_getIndex() {
+ return it.ll_getIndex();
+ }
+
+ @Override
+ public int ll_maxAnnotSpan() {
+ return it.ll_maxAnnotSpan();
+ }
+
+ @Override
+ public boolean isIndexesHaveBeenUpdated() {
+ return it.isIndexesHaveBeenUpdated();
+ }
+
+ @Override
+ public boolean maybeReinitIterator() {
+ return it.maybeReinitIterator();
+ }
+
+ @Override
+ public void moveToFirstNoReinit() {
+ moveToFirst();
+ }
+
+ @Override
+ public void moveToLastNoReinit() {
+ it.moveToLastNoReinit();
+ }
+
+ @Override
+ public void moveToNoReinit(FeatureStructure aFs) {
+ it.moveToNoReinit(aFs);
+ }
+
+ @Override
+ public Comparator<TOP> getComparator() {
+ return it.getComparator();
+ }
+ }
}
diff --git a/uimaj-core/src/test/java/org/apache/uima/cas/impl/SelectFsAssert.java b/uimaj-core/src/test/java/org/apache/uima/cas/impl/SelectFsAssert.java
index 8f8788fb..942d101 100644
--- a/uimaj-core/src/test/java/org/apache/uima/cas/impl/SelectFsAssert.java
+++ b/uimaj-core/src/test/java/org/apache/uima/cas/impl/SelectFsAssert.java
@@ -45,11 +45,10 @@ import org.apache.uima.jcas.tcas.Annotation;
import org.apache.uima.resource.metadata.TypeSystemDescription;
import org.apache.uima.util.CasCreationUtils;
import org.assertj.core.api.AutoCloseableSoftAssertions;
-import org.junit.Ignore;
public class SelectFsAssert {
-// private static final long RANDOM_SEED = System.nanoTime();
- private static final long RANDOM_SEED = 1174435820229231l;
+ private static final long RANDOM_SEED = System.nanoTime();
+// private static final long RANDOM_SEED = 1174435820229231l;
public static void assertSelectFS(RelativePosition aCondition, RelativeAnnotationPredicate aPredicate,
List<TestCase> aTestCases)
@@ -148,60 +147,12 @@ public class SelectFsAssert {
timings.compute("actual", (k, v) -> v == null ? 0l : v + currentTimeMillis() - tExpected);
try {
- {
- long tList = System.currentTimeMillis();
- List<Annotation> listActual = aActual.select(randomCas, typeX, y).asList();
- timings.compute("asList", (k, v) -> v == null ? 0l : v + currentTimeMillis() - tList);
- assertThat(listActual)
- .as("Selecting X of type [%s] %s [%s]@[%d-%d] asList%n%s%n", typeX.getName(), xRelToY,
- y.getType().getShortName(), y.getBegin(), y.getEnd(),
- casToString(randomCas))
- .containsExactlyElementsOf(expected);
- }
-
- // SELECT-ITERATORS: Leaving this for a latter improvement
-// {
-// long t = System.currentTimeMillis();
-// FSIterator<Annotation> it = aActual.select(randomCas, typeX, y).fsIterator();
-// Annotation initial = it.isValid() ? it.get() : null;
-// List<Annotation> actual = new ArrayList<>();
-// it.moveToFirst(); // <= This causes trouble
-// assertThat(it.isValid() ? it.get() : null)
-// .as("Annotation pointed at by iterator initially should match annotation after calling moveToFirst:%n"+
-// "%s%n%s%n" +
-// "Selecting X of type [%s] %s [%s]@[%d-%d] iterator forward%n%s%n",
-// initial, it.isValid() ? it.get() : null, typeX.getName(), xRelToY,
-// y.getType().getShortName(), y.getBegin(), y.getEnd(), casToString(randomCas))
-// .isEqualTo(initial);
-// while (it.isValid()) {
-// actual.add(it.get());
-// it.moveToNext();
-// }
-// timings.compute("iterator forward", (k, v) -> v == null ? 0l : v + currentTimeMillis() - t);
-// assertThat(actual)
-// .as("Selecting X of type [%s] %s [%s]@[%d-%d] iterator forward%n%s%n", typeX.getName(), xRelToY,
-// y.getType().getShortName(), y.getBegin(), y.getEnd(),
-// casToString(randomCas))
-// .containsExactlyElementsOf(expected);
-// }
-
- // SELECT-ITERATORS: Leaving this for a latter improvement
- // {
- // long tBackwards = System.currentTimeMillis();
- // FSIterator<Annotation> itBackwards = aActual.select(randomCas, type2, y).fsIterator();
- // List<Annotation> backwardsActual = new ArrayList<>();
- // itBackwards.moveToLast();
- // while (itBackwards.isValid()) {
- // backwardsActual.add(0, itBackwards.get());
- // itBackwards.moveToPrevious();
- // }
- // timings.compute("iterator backwards", (k, v) -> v == null ? 0l : v + currentTimeMillis() - tBackwards);
- // assertThat(backwardsActual)
- // .as("Selecting X of type [%s] %s [%s]@[%d-%d] iterator backwards%n%s%n", type2.getName(), xRelToY,
- // y.getType().getShortName(), y.getBegin(), y.getEnd(),
- // casToString(randomCas))
- // .containsExactlyElementsOf(expected);
- // }
+ assertSelectionAsList(expected, randomCas, aActual, xRelToY, typeX, typeY, y, timings);
+ assertSelectionAsForwardIteration(expected, randomCas, aActual, xRelToY, typeX, typeY,
+ y, timings);
+ // FIXME: Currently not working for all axes
+// assertSelectionAsBackwardIteration(expected, randomCas, aActual, xRelToY, typeX, typeY,
+// y, timings);
}
catch (Throwable e) {
// Set a breakpoint here to halt when an assert above fails. The select triggering the
@@ -227,6 +178,66 @@ public class SelectFsAssert {
}
}
+ private static void assertSelectionAsList(List<Annotation> expected, CAS randomCas,
+ TypeByContextSelectorAsSelection aActual,
+ String xRelToY, Type typeX, Type typeY, Annotation y, Map<String, Long> timings) {
+ long tList = System.currentTimeMillis();
+ List<Annotation> listActual = aActual.select(randomCas, typeX, y).asList();
+ timings.compute("asList", (k, v) -> v == null ? 0l : v + currentTimeMillis() - tList);
+ assertThat(listActual)
+ .as("Selecting X of type [%s] %s [%s]@[%d-%d] asList%n%s%n", typeX.getName(), xRelToY,
+ y.getType().getShortName(), y.getBegin(), y.getEnd(),
+ casToString(randomCas))
+ .containsExactlyElementsOf(expected);
+ }
+
+ private static void assertSelectionAsForwardIteration(List<Annotation> expected, CAS randomCas,
+ TypeByContextSelectorAsSelection aActual,
+ String xRelToY, Type typeX, Type typeY, Annotation y, Map<String, Long> timings) {
+ long t = System.currentTimeMillis();
+ FSIterator<Annotation> it = aActual.select(randomCas, typeX, y).fsIterator();
+ Annotation initial = it.isValid() ? it.get() : null;
+ List<Annotation> actual = new ArrayList<>();
+ it.moveToFirst();
+ assertThat(it.isValid() ? it.get() : null)
+ .as("Annotation pointed at by iterator initially should match annotation after calling "
+ + "moveToFirst:%n%s%n%s%n" +
+ "Selecting X of type [%s] %s [%s]@[%d-%d] iterator forward%n%s%n",
+ initial, it.isValid() ? it.get() : null, typeX.getName(), xRelToY,
+ y.getType().getShortName(), y.getBegin(), y.getEnd(), casToString(randomCas))
+ .isEqualTo(initial);
+ while (it.isValid()) {
+ actual.add(it.get());
+ it.moveToNext();
+ }
+ timings.compute("iterator forward", (k, v) -> v == null ? 0l : v + currentTimeMillis() - t);
+ assertThat(actual)
+ .as("Selecting X of type [%s] %s [%s]@[%d-%d] iterator forward%n%s%n", typeX.getName(),
+ xRelToY,
+ y.getType().getShortName(), y.getBegin(), y.getEnd(),
+ casToString(randomCas))
+ .containsExactlyElementsOf(expected);
+ }
+
+ private static void assertSelectionAsBackwardIteration(List<Annotation> expected, CAS randomCas,
+ TypeByContextSelectorAsSelection aActual,
+ String xRelToY, Type typeX, Type typeY, Annotation y, Map<String, Long> timings) {
+ long t = System.currentTimeMillis();
+ FSIterator<Annotation> it = aActual.select(randomCas, typeX, y).fsIterator();
+ List<Annotation> actual = new ArrayList<>();
+ it.moveToLast(); // <= This causes trouble
+ while (it.isValid()) {
+ actual.add(0, it.get());
+ it.moveToPrevious();
+ }
+ timings.compute("iterator backwards", (k, v) -> v == null ? 0l : v + currentTimeMillis() - t);
+ assertThat(actual)
+ .as("Selecting X of type [%s] %s [%s]@[%d-%d] iterator backwards%n%s%n", typeX.getName(), xRelToY,
+ y.getType().getShortName(), y.getBegin(), y.getEnd(),
+ casToString(randomCas))
+ .containsExactlyElementsOf(expected);
+ }
+
private static String casToString(CAS aCas) {
int MAX_ANNOTATIONS = 100;
if (aCas.select().count() > MAX_ANNOTATIONS) {
diff --git a/uimaj-core/src/test/java/org/apache/uima/cas/impl/SelectFsPredicateAlignmentTest.java b/uimaj-core/src/test/java/org/apache/uima/cas/impl/SelectFsPredicateAlignmentTest.java
index 915a52f..34a0e7b 100644
--- a/uimaj-core/src/test/java/org/apache/uima/cas/impl/SelectFsPredicateAlignmentTest.java
+++ b/uimaj-core/src/test/java/org/apache/uima/cas/impl/SelectFsPredicateAlignmentTest.java
@@ -334,5 +334,4 @@ public class SelectFsPredicateAlignmentTest {
}
return all;
}
-
}
diff --git a/uimaj-core/src/test/java/org/apache/uima/cas/impl/SelectFsTest.java b/uimaj-core/src/test/java/org/apache/uima/cas/impl/SelectFsTest.java
index e21f42d..83b84f3 100644
--- a/uimaj-core/src/test/java/org/apache/uima/cas/impl/SelectFsTest.java
+++ b/uimaj-core/src/test/java/org/apache/uima/cas/impl/SelectFsTest.java
@@ -706,7 +706,6 @@ public class SelectFsTest {
assertThat(it.isValid() ? it.get() : null).isSameAs(initial);
}
- @Ignore("SELECT-ITERATORS: Leaving this for a latter improvement")
@Test
public void thatSelectFollowingInitialPositionIsSameAsFirstPosition() throws Exception {
Annotation y = new Token(cas.getJCas(), 13, 34);
diff --git a/uimaj-core/src/test/java/org/apache/uima/cas/test/AnnotationIteratorTest.java b/uimaj-core/src/test/java/org/apache/uima/cas/test/AnnotationIteratorTest.java
index 935790b..b541385 100644
--- a/uimaj-core/src/test/java/org/apache/uima/cas/test/AnnotationIteratorTest.java
+++ b/uimaj-core/src/test/java/org/apache/uima/cas/test/AnnotationIteratorTest.java
@@ -246,29 +246,25 @@ public class AnnotationIteratorTest {
this.fss = afss;
isSave = fss.size() == 0; // on first call is 0, so save on first call
-// int count;
+ JCas jcas = cas.getJCas();
AnnotationIndex<Annotation> annotIndex = cas.getAnnotationIndex();
AnnotationIndex<Annotation> sentIndex = cas.getAnnotationIndex(sentenceType);
-
+
// assertTrue((isSave) ? it instanceof FSIteratorWrapper :
// FSIndexFlat.enabled ? it instanceof FSIndexFlat.FSIteratorFlat : it instanceof FSIteratorWrapper);
assertCount("Normal ambiguous annot iterator", annotCount, annotIndex.iterator(true));
- // a normal "ambiguous" iterator
- assertCount("Normal ambiguous select annot iterator", annotCount, annotIndex.select().fsIterator());
+
+ assertCount("Normal ambiguous select annot iterator", annotCount,
+ annotIndex.select().fsIterator());
assertEquals(annotCount, annotIndex.select().toArray().length); // stream op
assertEquals(annotCount, annotIndex.select().asArray(Annotation.class).length); // select op
assertEquals(annotCount - 5, annotIndex.select().startAt(2).asArray(Annotation.class).length);
-
- Annotation[] tokensAndSentencesAndPhrases = annotIndex.select().asArray(Annotation.class);
-
- JCas jcas = cas.getJCas();
-
- FSArray<Annotation> fsa = FSArray.create(jcas, tokensAndSentencesAndPhrases);
- NonEmptyFSList<Annotation> fslhead = (NonEmptyFSList<Annotation>) FSList.<Annotation, Annotation>create(jcas, tokensAndSentencesAndPhrases);
-
+
+ FSArray<Annotation> fsa = FSArray.create(jcas, annotIndex.select().asArray(Annotation.class));
assertCount("fsa ambiguous select annot iterator", annotCount,
fsa.select().fsIterator());
+ NonEmptyFSList<Annotation> fslhead = (NonEmptyFSList<Annotation>) FSList.<Annotation, Annotation>create(jcas, annotIndex.select().asArray(Annotation.class));
assertCount("fslhead ambiguous select annot iterator", annotCount,
fslhead.select().fsIterator());