gsmiller commented on code in PR #1004:
URL: https://github.com/apache/lucene/pull/1004#discussion_r914131349
##########
lucene/memory/src/test/org/apache/lucene/index/memory/TestMemoryIndex.java:
##########
@@ -298,10 +298,10 @@ public void testDocValues() throws Exception {
assertEquals(3, sortedSetDocValues.getValueCount());
assertEquals(0, sortedSetDocValues.nextDoc());
assertEquals(3, sortedSetDocValues.docValueCount());
+ assertEquals(3, sortedSetDocValues.docValueCount());
Review Comment:
Looks like we're already asserting this on the previous line :)
##########
lucene/core/src/test/org/apache/lucene/index/TestSortedSetDocValues.java:
##########
@@ -1,26 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to You under the Apache License, Version 2.0
- * (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.apache.lucene.index;
-
-import org.apache.lucene.tests.util.LuceneTestCase;
-
-public class TestSortedSetDocValues extends LuceneTestCase {
-
- public void testNoMoreOrdsConstant() {
Review Comment:
Let's keep this test as long as we still define `NO_MORE_ORDS`. Even though
we've marked it as deprecated and are moving off of using it for iteration, our
users are likely still relying on it for iteration so we should still keep this
test. We can remove it at the same time we actually remove the constant
definition.
##########
lucene/test-framework/src/java/org/apache/lucene/tests/index/BaseDocValuesFormatTestCase.java:
##########
@@ -1878,14 +1878,12 @@ public void testSortedSetTwoDocumentsMerged() throws
IOException {
assertEquals(0, dv.nextDoc());
assertEquals(0, dv.nextOrd());
- assertEquals(NO_MORE_ORDS, dv.nextOrd());
Review Comment:
Should we assert the docValueCount is 1 here?
##########
lucene/test-framework/src/java/org/apache/lucene/tests/util/LuceneTestCase.java:
##########
@@ -2576,11 +2576,11 @@ public void assertDocValuesEquals(String info,
IndexReader leftReader, IndexRead
if (docID == NO_MORE_DOCS) {
break;
}
- long ord;
- while ((ord = leftValues.nextOrd()) !=
SortedSetDocValues.NO_MORE_ORDS) {
+ assertEquals(info, leftValues.docValueCount(),
rightValues.docValueCount());
+ for (int i = 0; i < leftValues.docValueCount(); i++) {
+ long ord = leftValues.nextOrd();
Review Comment:
minor: I might just one-line the for-loop body to: `assertEquals(info,
leftValues.nextOrd(), rightValues.nextOrd());` Alternatively, if you find it
more readable to create the local variables, I'd create one for each:
```
long leftOrd = leftValues.nextOrd();
long rightOrd = rightValues.nextOrd();
assertEquals(info, leftOrd, rightOrd);
```
Just feels a little inconsistent as it currently is :)
##########
lucene/backward-codecs/src/test/org/apache/lucene/backward_codecs/lucene80/BaseLucene80DocValuesFormatTestCase.java:
##########
@@ -480,15 +476,14 @@ public void testSortedSetAroundBlockSize() throws
IOException {
for (int i = 0; i < maxDoc; ++i) {
assertEquals(i, values.nextDoc());
final int numValues = in.readVInt();
+ assertEquals(numValues, values.docValueCount());
for (int j = 0; j < numValues; ++j) {
b.setLength(in.readVInt());
b.grow(b.length());
in.readBytes(b.bytes(), 0, b.length());
assertEquals(b.get(), values.lookupOrd(values.nextOrd()));
}
-
- assertEquals(SortedSetDocValues.NO_MORE_ORDS, values.nextOrd());
Review Comment:
I think we ought to keep this for now until we actually remove the contract
that `nextOrd()` returns `NO_MORE_ORDS` when exhausted (assuming we plan to
back-port this to 9.x, which I think we should). Since 9.x will need to
continue to return `NO_MORE_ORDS` as part of the API contract, it would be good
to have tests for that behavior. When we go to actually remove `NO_MORE_ORDS`,
which we should do only to `main` and under a separate Jira issue, we can
remove this check.
##########
lucene/backward-codecs/src/test/org/apache/lucene/backward_index/TestBackwardsCompatibility.java:
##########
@@ -1205,8 +1205,8 @@ public void searchIndex(
assertEquals(id, dvShort.longValue());
assertEquals(i, dvSortedSet.nextDoc());
+ assertEquals(1, dvSortedSet.docValueCount());
long ord = dvSortedSet.nextOrd();
- assertEquals(SortedSetDocValues.NO_MORE_ORDS, dvSortedSet.nextOrd());
Review Comment:
Here's another place where I think we should keep the check for now.
##########
lucene/memory/src/test/org/apache/lucene/index/memory/TestMemoryIndexAgainstDirectory.java:
##########
@@ -533,15 +533,13 @@ public void testDocValuesMemoryIndexVsNormalIndex()
throws Exception {
controlLeafReader.getSortedSetDocValues("sorted_set");
assertEquals(0, controlSortedSetDocValues.nextDoc());
assertEquals(controlSortedSetDocValues.getValueCount(),
sortedSetDocValues.getValueCount());
- for (long controlOrd = controlSortedSetDocValues.nextOrd();
- controlOrd != SortedSetDocValues.NO_MORE_ORDS;
- controlOrd = controlSortedSetDocValues.nextOrd()) {
+ for (int i = 0; i < controlSortedDocValues.getValueCount(); i++) {
+ long controlOrd = controlSortedSetDocValues.nextOrd();
assertEquals(controlOrd, sortedSetDocValues.nextOrd());
assertEquals(
controlSortedSetDocValues.lookupOrd(controlOrd),
sortedSetDocValues.lookupOrd(controlOrd));
}
- assertEquals(SortedSetDocValues.NO_MORE_ORDS,
sortedSetDocValues.nextOrd());
Review Comment:
Keep for now?
##########
lucene/test-framework/src/java/org/apache/lucene/tests/index/BaseDocValuesFormatTestCase.java:
##########
@@ -16,7 +16,6 @@
*/
package org.apache.lucene.tests.index;
-import static org.apache.lucene.index.SortedSetDocValues.NO_MORE_ORDS;
Review Comment:
For the same reason given elsewhere, let's keep the NO_MORE_ORDS checks for
now so we can back-port. These will all be easy to just remove when we remove
the definition of NO_MORE_ORDS.
##########
lucene/test-framework/src/java/org/apache/lucene/tests/index/BaseDocValuesFormatTestCase.java:
##########
@@ -1878,14 +1878,12 @@ public void testSortedSetTwoDocumentsMerged() throws
IOException {
assertEquals(0, dv.nextDoc());
assertEquals(0, dv.nextOrd());
- assertEquals(NO_MORE_ORDS, dv.nextOrd());
BytesRef bytes = dv.lookupOrd(0);
assertEquals(newBytesRef("hello"), bytes);
assertEquals(1, dv.nextDoc());
assertEquals(1, dv.nextOrd());
- assertEquals(NO_MORE_ORDS, dv.nextOrd());
Review Comment:
... and here too?
##########
lucene/core/src/test/org/apache/lucene/codecs/lucene90/TestLucene90DocValuesFormat.java:
##########
@@ -497,8 +492,6 @@ public void testSortedSetAroundBlockSize() throws
IOException {
in.readBytes(b.bytes(), 0, b.length());
assertEquals(b.get(), values.lookupOrd(values.nextOrd()));
}
-
- assertEquals(SortedSetDocValues.NO_MORE_ORDS, values.nextOrd());
Review Comment:
For reasons I gave above, this is another check I think we should keep for
now, assuming we plan to back-port.
##########
lucene/memory/src/test/org/apache/lucene/index/memory/TestMemoryIndex.java:
##########
@@ -298,10 +298,10 @@ public void testDocValues() throws Exception {
assertEquals(3, sortedSetDocValues.getValueCount());
assertEquals(0, sortedSetDocValues.nextDoc());
assertEquals(3, sortedSetDocValues.docValueCount());
+ assertEquals(3, sortedSetDocValues.docValueCount());
assertEquals(0L, sortedSetDocValues.nextOrd());
assertEquals(1L, sortedSetDocValues.nextOrd());
assertEquals(2L, sortedSetDocValues.nextOrd());
- assertEquals(SortedSetDocValues.NO_MORE_ORDS,
sortedSetDocValues.nextOrd());
Review Comment:
For the same reason I've given elsewhere, I think we should keep this check
for now.
##########
lucene/memory/src/test/org/apache/lucene/index/memory/TestMemoryIndex.java:
##########
@@ -329,10 +329,10 @@ public void testDocValues_resetIterator() throws
Exception {
assertEquals(3, sortedSetDocValues.getValueCount());
for (int times = 0; times < 3; times++) {
assertTrue(sortedSetDocValues.advanceExact(0));
+ assertEquals(3, sortedSetDocValues.docValueCount());
assertEquals(0L, sortedSetDocValues.nextOrd());
assertEquals(1L, sortedSetDocValues.nextOrd());
assertEquals(2L, sortedSetDocValues.nextOrd());
- assertEquals(SortedSetDocValues.NO_MORE_ORDS,
sortedSetDocValues.nextOrd());
Review Comment:
Keep for now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
