Skip to content
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.

Sign up for GitHub

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

Prevent users from using document block APIs when sort is configured #12711

Closed
wants to merge 5 commits into from
Closed

Prevent users from using document block APIs when sort is configured #12711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5352979
initial commit
s1monw Oct 19, 2023
35a448d
add tests
s1monw Oct 20, 2023
51fbb7e
Merge branch 'main' into no_blocks_with_sorting
s1monw Oct 23, 2023
29497e4
format
s1monw Oct 23, 2023
aef6ea2
only one return statement
s1monw Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions lucene/core/src/java/org/apache/lucene/index/DocumentsWriterPerThread.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,11 @@ long updateDocuments(
boolean allDocsIndexed = false;
try {
for (Iterable<? extends IndexableField> doc : docs) {
if (docsInRamBefore != numDocsInRAM && indexWriterConfig.getIndexSort() != null) {
throw new IllegalStateException(
"Can't use index API with multiple documents when index sorting is used. Sort: "
+ indexWriterConfig.getIndexSort());
}
// Even on exception, the document is still added (but marked
// deleted), so we don't need to un-reserve at that point.
// Aborting exceptions will actually "lose" more than one
Expand All @@ -246,9 +251,6 @@ long updateDocuments(
}
}
allDocsIndexed = true;
if (numDocsInRAM - docsInRamBefore > 1) {
segmentInfo.setHasBlocks();
}
return finishDocuments(deleteNode, docsInRamBefore);
} finally {
if (!allDocsIndexed && !aborted) {
Expand Down Expand Up @@ -279,7 +281,6 @@ private long finishDocuments(DocumentsWriterDeleteQueue.Node<?> deleteNode, int
seqNo = deleteQueue.add(deleteNode, deleteSlice);
assert deleteSlice.isTail(deleteNode) : "expected the delete term as the tail item";
deleteSlice.apply(pendingUpdates, docIdUpTo);
return seqNo;
} else {
seqNo = deleteQueue.updateSlice(deleteSlice);
if (seqNo < 0) {
Expand All @@ -289,7 +290,9 @@ private long finishDocuments(DocumentsWriterDeleteQueue.Node<?> deleteNode, int
deleteSlice.reset();
}
}

if (numDocsInRAM - docIdUpTo > 1) {
segmentInfo.setHasBlocks();
}
return seqNo;
}

Expand Down
69 changes: 48 additions & 21 deletions lucene/core/src/test/org/apache/lucene/index/TestIndexWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,28 +56,9 @@
import org.apache.lucene.analysis.tokenattributes.PositionIncrementAttribute;
import org.apache.lucene.codecs.Codec;
import org.apache.lucene.codecs.simpletext.SimpleTextCodec;
import org.apache.lucene.document.BinaryDocValuesField;
import org.apache.lucene.document.Document;
import org.apache.lucene.document.Field;
import org.apache.lucene.document.FieldType;
import org.apache.lucene.document.LongPoint;
import org.apache.lucene.document.NumericDocValuesField;
import org.apache.lucene.document.SortedDocValuesField;
import org.apache.lucene.document.SortedNumericDocValuesField;
import org.apache.lucene.document.SortedSetDocValuesField;
import org.apache.lucene.document.StoredField;
import org.apache.lucene.document.StringField;
import org.apache.lucene.document.TextField;
import org.apache.lucene.document.*;
import org.apache.lucene.index.IndexWriterConfig.OpenMode;
import org.apache.lucene.search.DocIdSetIterator;
import org.apache.lucene.search.IndexSearcher;
import org.apache.lucene.search.MatchAllDocsQuery;
import org.apache.lucene.search.PhraseQuery;
import org.apache.lucene.search.ScoreDoc;
import org.apache.lucene.search.SearcherFactory;
import org.apache.lucene.search.SearcherManager;
import org.apache.lucene.search.TermQuery;
import org.apache.lucene.search.TopDocs;
import org.apache.lucene.search.*;
import org.apache.lucene.store.AlreadyClosedException;
import org.apache.lucene.store.ByteBuffersDirectory;
import org.apache.lucene.store.Directory;
Expand Down Expand Up @@ -4857,4 +4838,50 @@ private static void addDocWithField(IndexWriter writer, String field) throws IOE
doc.add(newField(field, "value", storedTextType));
writer.addDocument(doc);
}

public void testUseBlockAPIWithIndexSort() throws IOException {
try (Directory dir = newDirectory()) {
Sort sort = new Sort(new SortField("foo", SortField.Type.LONG));
try (IndexWriter writer = new IndexWriter(dir, new IndexWriterConfig().setIndexSort(sort))) {
writer.addDocument(new Document()); // add a doc... no aborting exceptions should be thrown
expectThrows(
IllegalStateException.class,
() -> writer.addDocuments(Arrays.asList(new Document(), new Document())));

String msg =
"Can't use index API with multiple documents when index sorting is used. Sort: <long: \"foo\">";
assertEquals(
msg,
expectThrows(
IllegalStateException.class,
() ->
writer.updateDocuments(
new Term("foo", "bar"), Arrays.asList(new Document(), new Document())))
.getMessage());
assertEquals(
msg,
expectThrows(
IllegalStateException.class,
() ->
writer.softUpdateDocuments(
new Term("foo", "bar"),
Arrays.asList(new Document(), new Document()),
new NumericDocValuesField("id", 1)))
.getMessage());
assertEquals(
msg,
expectThrows(
IllegalStateException.class,
() ->
writer.updateDocuments(
new TermQuery(new Term("foo", "bar")),
Arrays.asList(new Document(), new Document())))
.getMessage());
writer.commit();
try (DirectoryReader reader = DirectoryReader.open(dir)) {
assertEquals(1, reader.numDocs());
}
}
}
}
}
8 changes: 6 additions & 2 deletions lucene/core/src/test/org/apache/lucene/search/TestTopFieldCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,9 +184,13 @@ public void testTotalHits() throws Exception {
dir, newIndexWriterConfig().setMergePolicy(NoMergePolicy.INSTANCE).setIndexSort(sort));
Document doc = new Document();
doc.add(new NumericDocValuesField("foo", 3));
w.addDocuments(Arrays.asList(doc, doc, doc, doc));
for (int i = 0; i < 4; i++) {
w.addDocument(doc);
}
w.flush();
w.addDocuments(Arrays.asList(doc, doc, doc, doc, doc, doc));
for (int i = 0; i < 6; i++) {
w.addDocument(doc);
}
w.flush();
IndexReader reader = DirectoryReader.open(w);
assertEquals(2, reader.leaves().size());
Expand Down
33 changes: 1 addition & 32 deletions lucene/test-framework/src/java/org/apache/lucene/tests/index/RandomIndexWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import java.io.Closeable;
import java.io.IOException;
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;
import java.util.Random;
import java.util.concurrent.CopyOnWriteArrayList;
Expand Down Expand Up @@ -188,37 +187,7 @@ public <T extends IndexableField> long addDocument(final Iterable<T> doc) throws
// (but we need to clone them), and only when
// getReader, commit, etc. are called, we do an
// addDocuments? Would be better testing.
seqNo =
w.addDocuments(
new Iterable<Iterable<T>>() {

@Override
public Iterator<Iterable<T>> iterator() {
return new Iterator<Iterable<T>>() {

boolean done;

@Override
public boolean hasNext() {
return !done;
}

@Override
public void remove() {
throw new UnsupportedOperationException();
}

@Override
public Iterable<T> next() {
if (done) {
throw new IllegalStateException();
}
done = true;
return doc;
}
};
}
});
seqNo = w.addDocuments(List.of(doc));
} else {
seqNo = w.addDocument(doc);
}
Expand Down
Loading