Skip to content
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,11 @@ public BlockLoader.Block tryRead(
boolean toInt,
boolean binaryMultiValuedFormat
) throws IOException {
if (docs.mayContainDuplicates()) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can go further even with duplicates, but Martijn or Parker can follow up on it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can look into removing these if statements in follow ups.

// isCompressed assumes there aren't duplicates
return null;
}

int count = docs.count() - offset;
int firstDocId = docs.get(offset);
int lastDocId = docs.get(docs.count() - 1);
Expand Down Expand Up @@ -452,6 +457,11 @@ public BlockLoader.Block tryRead(
boolean toInt,
boolean binaryMultiValuedFormat
) throws IOException {
if (docs.mayContainDuplicates()) {
// isCompressed assumes there aren't duplicates
return null;
}

int count = docs.count() - offset;
int firstDocId = docs.get(offset);
int lastDocId = docs.get(docs.count() - 1);
Expand Down Expand Up @@ -2105,6 +2115,10 @@ public BlockLoader.Block tryRead(

@Override
BlockLoader.Block tryRead(BlockLoader.SingletonLongBuilder builder, BlockLoader.Docs docs, int offset) throws IOException {
if (docs.mayContainDuplicates()) {
// isCompressed assumes there aren't duplicates
return null;
}
final int docsCount = docs.count();
doc = docs.get(docsCount - 1);
for (int i = offset; i < docsCount;) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@
* spent the past 40 years making them really really good at running tight loops over
* arrays of data. So we play along with the CPU and make arrays.
* </p>
* <p>
* Implementers will return non-null from either {@link #columnAtATimeReader} or
* {@link #rowStrideReader}. As of 2026-2-4 many will return non-null from both
* but that's deprecated and will be removed.
* </p>
* <h2>How to implement</h2>
* <p>
* There are a lot of interesting choices hiding in here to make getting those arrays
Expand Down Expand Up @@ -253,6 +258,11 @@ interface RowStrideReader extends Reader {
void read(int docId, StoredFields storedFields, Builder builder) throws IOException;
}

/**
* @deprecated we no longer need to implement {@link RowStrideReader} when
* storage prefers column-at-a-time
*/
@Deprecated
interface AllReader extends ColumnAtATimeReader, RowStrideReader {}

interface StoredFields {
Expand Down Expand Up @@ -293,16 +303,17 @@ interface StoredFields {
* Build a column-at-a-time reader. <strong>May</strong> return {@code null}
* if the underlying storage needs to be loaded row-by-row. Callers should try
* this first, only falling back to {@link #rowStrideReader} if this returns
* {@code null} or if they can't load column-at-a-time themselves.
* {@code null}. If this returns null then {@link #rowStrideReader} may not.
*/
@Nullable
IOSupplier<ColumnAtATimeReader> columnAtATimeReader(LeafReaderContext context) throws IOException;

/**
* Build a row-by-row reader. Must <strong>never</strong> return {@code null},
* evan if the underlying storage prefers to be loaded column-at-a-time. Some
* callers simply can't load column-at-a-time so all implementations must support
* this method.
* Build a row-by-row reader. <strong>May</strong> return {@code null} if the
* underlying storage prefers to be loaded column-at-a-time. Callers should try
* {@link #columnAtATimeReader} first, only falling back to this if
* {@link #columnAtATimeReader} returns null. This may not return null if
* {@link #columnAtATimeReader} does.
*/
RowStrideReader rowStrideReader(LeafReaderContext context) throws IOException;

Expand Down Expand Up @@ -429,6 +440,30 @@ interface Docs {
int count();

int get(int i);

/**
* Can this vector reference duplicate documents? Some {@link BlockLoader}s will
* run more slowly if this is {@code true}. These {@linkplain BlockLoader}s will
* return incorrect results if there are duplicates and this is {@code false}.
* This exists because of a hierarchy of speeds:
* <ul>
* <li>
* We can better optimize some {@link BlockLoader}s when they receive
* {@linkplain Docs}s that don't contain duplicates.
* </li>
* <li>
* It's rare that we want to load from duplicate doc ids. We don't need
* to spend that much time optimizing it.
* </li>
* <li>
* We sometimes really <strong>want</strong> to load from duplicate
* doc ids to minimize total amount of loading we have to do in fairly
* specific cases like resolving dimension values after time series
* aggregations.
* </li>
* </ul>
*/
boolean mayContainDuplicates();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick look indicates to most implementations return false here. Maybe have a default implementation that returns false?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it but figured it was kinder to make the implementer think about the choice when implementing.

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2073,17 +2073,7 @@ public DocValuesFormat getDocValuesFormatForField(String field) {
var factory = TestBlock.factory();
try (DirectoryReader reader = DirectoryReader.open(writer)) {
for (LeafReaderContext leaf : reader.leaves()) {
BlockLoader.Docs docs = new BlockLoader.Docs() {
@Override
public int count() {
return leaf.reader().maxDoc();
}

@Override
public int get(int i) {
return i;
}
};
BlockLoader.Docs docs = TestBlock.docs(leaf);
var idReader = ESTestCase.asInstanceOf(OptionalColumnAtATimeReader.class, leaf.reader().getNumericDocValues("id"));
TestBlock idBlock = (TestBlock) idReader.tryRead(factory, docs, 0, false, null, false, false);
assertNotNull(idBlock);
Expand Down Expand Up @@ -2151,6 +2141,11 @@ public int count() {
public int get(int docId) {
return docId;
}

@Override
public boolean mayContainDuplicates() {
return false;
}
}, start);
assertNotNull(hostBlock);
assertThat(hostBlock.size(), equalTo(end - start + 1));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,17 +127,7 @@ public void testNullValueBlockLoader() throws IOException {
try (DirectoryReader reader = DirectoryReader.open(directory)) {
TestBlock block = (TestBlock) loader.columnAtATimeReader(reader.leaves().get(0))
.get()
.read(TestBlock.factory(), new BlockLoader.Docs() {
@Override
public int count() {
return 1;
}

@Override
public int get(int i) {
return 0;
}
}, 0, false);
.read(TestBlock.factory(), TestBlock.docs(0), 0, false);
assertThat(block.get(0), nullValue());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,8 @@ public BlockLoader.TDigestBuilder tdigestBlockBuilder(int count) {
};
}

public static final BlockLoader.Docs docs(int... docs) {
public static BlockLoader.Docs docs(int... docs) {
boolean containsDupes = containsDupes(docs);
return new BlockLoader.Docs() {
@Override
public int count() {
Expand All @@ -585,10 +586,20 @@ public int count() {
public int get(int i) {
return docs[i];
}

@Override
public boolean mayContainDuplicates() {
return containsDupes;
}

@Override
public String toString() {
return "docs " + Arrays.toString(docs);
}
};
}

public static final BlockLoader.Docs docs(LeafReaderContext ctx) {
public static BlockLoader.Docs docs(LeafReaderContext ctx) {
return new BlockLoader.Docs() {
@Override
public int count() {
Expand All @@ -599,6 +610,40 @@ public int count() {
public int get(int i) {
return i;
}

@Override
public boolean mayContainDuplicates() {
return false;
}

@Override
public String toString() {
return "docs for " + ctx;
}
};
}

public static BlockLoader.Docs docsUpTo(int end) {
return new BlockLoader.Docs() {
@Override
public int count() {
return end;
}

@Override
public int get(int i) {
return i;
}

@Override
public boolean mayContainDuplicates() {
return false;
}

@Override
public String toString() {
return "range to " + end;
}
};
}

Expand Down Expand Up @@ -982,5 +1027,18 @@ public BlockLoader.LongBuilder appendLong(long value) {
return this;
}
}
};
}

static boolean containsDupes(int[] docs) {
int[] sorted = Arrays.copyOf(docs, docs.length);
int prev = sorted[0];
for (int i = 1; i < docs.length; i++) {
int v = sorted[i];
if (prev == v) {
return true;
}
prev = v;
}
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ class ComputeBlockLoaderFactory extends DelegatingBlockLoaderFactory implements
public Block constantNulls(int count) {
if (nullBlock == null) {
nullBlock = factory.newConstantNullBlock(count);
} else {
if (nullBlock.getPositionCount() != count) {
nullBlock.close();
nullBlock = factory.newConstantNullBlock(count);
}
}
nullBlock.incRef();
return nullBlock;
Expand Down
Loading