Skip to content

Commit

Permalink
Merge remote-tracking branch 'upstream/main' into collapse_with_rescore
Browse files Browse the repository at this point in the history
  • Loading branch information
jimczi committed Apr 24, 2024
2 parents 655b04d + d34f0b1 commit f448548
Show file tree
Hide file tree
Showing 38 changed files with 491 additions and 63 deletions.
6 changes: 6 additions & 0 deletions docs/changelog/107577.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 107577
summary: "ESQL: Fix MV_DEDUPE when using data from an index"
area: ES|QL
type: bug
issues:
- 104745
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ static TransportVersion def(int id) {
public static final TransportVersion TOP_LEVEL_KNN_SUPPORT_QUERY_NAME = def(8_641_00_0);
public static final TransportVersion INDEX_SEGMENTS_VECTOR_FORMATS = def(8_642_00_0);
public static final TransportVersion ADD_RESOURCE_ALREADY_UPLOADED_EXCEPTION = def(8_643_00_0);
public static final TransportVersion ESQL_MV_ORDERING_SORTED_ASCENDING = def(8_644_00_0);

/*
* STOP! READ THIS FIRST! No, really,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ interface Docs {
interface BlockFactory {
/**
* Build a builder to load booleans as loaded from doc values. Doc values
* load booleans deduplicated and in sorted order.
* load booleans in sorted order.
*/
BooleanBuilder booleansFromDocValues(int expectedCount);

Expand All @@ -353,7 +353,7 @@ interface BlockFactory {

/**
* Build a builder to load doubles as loaded from doc values.
* Doc values load doubles deduplicated and in sorted order.
* Doc values load doubles in sorted order.
*/
DoubleBuilder doublesFromDocValues(int expectedCount);

Expand All @@ -364,7 +364,7 @@ interface BlockFactory {

/**
* Build a builder to load ints as loaded from doc values.
* Doc values load ints deduplicated and in sorted order.
* Doc values load ints in sorted order.
*/
IntBuilder intsFromDocValues(int expectedCount);

Expand All @@ -375,7 +375,7 @@ interface BlockFactory {

/**
* Build a builder to load longs as loaded from doc values.
* Doc values load longs deduplicated and in sorted order.
* Doc values load longs in sorted order.
*/
LongBuilder longsFromDocValues(int expectedCount);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,12 @@ protected void doAssertLuceneQuery(FunctionScoreQueryBuilder queryBuilder, Query
}
}

@Override
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/107782")
public void testToQuery() throws IOException {
super.testToQuery();
}

public void testIllegalArguments() {
expectThrows(IllegalArgumentException.class, () -> new FunctionScoreQueryBuilder((QueryBuilder) null));
expectThrows(IllegalArgumentException.class, () -> new FunctionScoreQueryBuilder((ScoreFunctionBuilder<?>) null));
Expand Down
5 changes: 5 additions & 0 deletions x-pack/plugin/esql/build.gradle
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import org.elasticsearch.gradle.internal.info.BuildParams
import org.elasticsearch.gradle.internal.precommit.CheckForbiddenApisTask;

apply plugin: 'elasticsearch.internal-es-plugin'
apply plugin: 'elasticsearch.internal-cluster-test'
Expand Down Expand Up @@ -292,3 +293,7 @@ tasks.named('stringTemplates').configure {
it.outputFile = "org/elasticsearch/xpack/esql/enrich/EnrichResultBuilderForBoolean.java"
}
}

tasks.withType(CheckForbiddenApisTask).configureEach {
signaturesFiles += files('src/main/resources/forbidden/ql-signatures.txt')
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

package org.elasticsearch.compute.data;

import org.elasticsearch.TransportVersions;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.core.Nullable;
Expand Down Expand Up @@ -178,6 +179,11 @@ void writeSubFields(StreamOutput out) throws IOException {
if (nullsMask != null) {
out.writeLongArray(nullsMask.toLongArray());
}
out.writeEnum(mvOrdering);
if (out.getTransportVersion().before(TransportVersions.ESQL_MV_ORDERING_SORTED_ASCENDING)
&& mvOrdering == MvOrdering.SORTED_ASCENDING) {
out.writeEnum(MvOrdering.UNORDERED);
} else {
out.writeEnum(mvOrdering);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ public interface Block extends Accountable, BlockLoader.Block, NamedWriteable, R
enum MvOrdering {
UNORDERED(false, false),
DEDUPLICATED_UNORDERD(true, false),
DEDUPLICATED_AND_SORTED_ASCENDING(true, true);
DEDUPLICATED_AND_SORTED_ASCENDING(true, true),
SORTED_ASCENDING(false, true);

private final boolean deduplicated;
private final boolean sortedAscending;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,7 @@ private ComputeBlockLoaderFactory(BlockFactory factory, int pageSize) {

@Override
public BlockLoader.BooleanBuilder booleansFromDocValues(int expectedCount) {
return factory.newBooleanBlockBuilder(expectedCount).mvOrdering(Block.MvOrdering.DEDUPLICATED_AND_SORTED_ASCENDING);
return factory.newBooleanBlockBuilder(expectedCount).mvOrdering(Block.MvOrdering.SORTED_ASCENDING);
}

@Override
Expand All @@ -600,7 +600,7 @@ public BlockLoader.BytesRefBuilder bytesRefs(int expectedCount) {

@Override
public BlockLoader.DoubleBuilder doublesFromDocValues(int expectedCount) {
return factory.newDoubleBlockBuilder(expectedCount).mvOrdering(Block.MvOrdering.DEDUPLICATED_AND_SORTED_ASCENDING);
return factory.newDoubleBlockBuilder(expectedCount).mvOrdering(Block.MvOrdering.SORTED_ASCENDING);
}

@Override
Expand All @@ -610,7 +610,7 @@ public BlockLoader.DoubleBuilder doubles(int expectedCount) {

@Override
public BlockLoader.IntBuilder intsFromDocValues(int expectedCount) {
return factory.newIntBlockBuilder(expectedCount).mvOrdering(Block.MvOrdering.DEDUPLICATED_AND_SORTED_ASCENDING);
return factory.newIntBlockBuilder(expectedCount).mvOrdering(Block.MvOrdering.SORTED_ASCENDING);
}

@Override
Expand All @@ -620,7 +620,7 @@ public BlockLoader.IntBuilder ints(int expectedCount) {

@Override
public BlockLoader.LongBuilder longsFromDocValues(int expectedCount) {
return factory.newLongBlockBuilder(expectedCount).mvOrdering(Block.MvOrdering.DEDUPLICATED_AND_SORTED_ASCENDING);
return factory.newLongBlockBuilder(expectedCount).mvOrdering(Block.MvOrdering.SORTED_ASCENDING);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public MultivalueDedupeBoolean(BooleanBlock block) {
* Dedupe values using an adaptive algorithm based on the size of the input list.
*/
public BooleanBlock dedupeToBlock(BlockFactory blockFactory) {
if (false == block.mayHaveMultivaluedFields()) {
if (block.mvDeduplicated()) {
block.incRef();
return block;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,7 @@ public void testLoadAll() {
loadSimpleAndAssert(
driverContext,
CannedSourceOperator.collectPages(simpleInput(driverContext.blockFactory(), between(100, 5000))),
Block.MvOrdering.SORTED_ASCENDING,
Block.MvOrdering.DEDUPLICATED_AND_SORTED_ASCENDING
);
}
Expand All @@ -416,6 +417,7 @@ public void testLoadAllInOnePage() {
CannedSourceOperator.collectPages(simpleInput(driverContext.blockFactory(), between(100, 5000)))
)
),
Block.MvOrdering.UNORDERED,
Block.MvOrdering.UNORDERED
);
}
Expand All @@ -426,7 +428,7 @@ public void testManySingleDocPages() {
List<Page> input = CannedSourceOperator.collectPages(simpleInput(driverContext, numDocs, between(1, numDocs), 1));
Randomness.shuffle(input);
List<Operator> operators = new ArrayList<>();
Checks checks = new Checks(Block.MvOrdering.DEDUPLICATED_AND_SORTED_ASCENDING);
Checks checks = new Checks(Block.MvOrdering.DEDUPLICATED_AND_SORTED_ASCENDING, Block.MvOrdering.DEDUPLICATED_AND_SORTED_ASCENDING);
FieldCase testCase = new FieldCase(
new KeywordFieldMapper.KeywordFieldType("kwd"),
ElementType.BYTES_REF,
Expand Down Expand Up @@ -457,6 +459,7 @@ public void testEmpty() {
loadSimpleAndAssert(
driverContext,
CannedSourceOperator.collectPages(simpleInput(driverContext.blockFactory(), 0)),
Block.MvOrdering.UNORDERED,
Block.MvOrdering.UNORDERED
);
}
Expand All @@ -475,7 +478,7 @@ public void testLoadAllInOnePageShuffled() {
shuffledBlocks[b] = source.getBlock(b).filter(shuffleArray);
}
source = new Page(shuffledBlocks);
loadSimpleAndAssert(driverContext, List.of(source), Block.MvOrdering.UNORDERED);
loadSimpleAndAssert(driverContext, List.of(source), Block.MvOrdering.UNORDERED, Block.MvOrdering.UNORDERED);
}

private static ValuesSourceReaderOperator.FieldInfo fieldInfo(MappedFieldType ft, ElementType elementType) {
Expand Down Expand Up @@ -521,8 +524,13 @@ public FieldNamesFieldMapper.FieldNamesFieldType fieldNames() {
};
}

private void loadSimpleAndAssert(DriverContext driverContext, List<Page> input, Block.MvOrdering docValuesMvOrdering) {
List<FieldCase> cases = infoAndChecksForEachType(docValuesMvOrdering);
private void loadSimpleAndAssert(
DriverContext driverContext,
List<Page> input,
Block.MvOrdering booleanAndNumericalDocValuesMvOrdering,
Block.MvOrdering bytesRefDocValuesMvOrdering
) {
List<FieldCase> cases = infoAndChecksForEachType(booleanAndNumericalDocValuesMvOrdering, bytesRefDocValuesMvOrdering);

List<Operator> operators = new ArrayList<>();
operators.add(
Expand Down Expand Up @@ -621,7 +629,10 @@ private void testLoadAllStatus(boolean allInOnePage) {
List<Page> input = CannedSourceOperator.collectPages(simpleInput(driverContext, numDocs, commitEvery(numDocs), numDocs));
assertThat(reader.leaves(), hasSize(10));
assertThat(input, hasSize(10));
List<FieldCase> cases = infoAndChecksForEachType(Block.MvOrdering.DEDUPLICATED_AND_SORTED_ASCENDING);
List<FieldCase> cases = infoAndChecksForEachType(
Block.MvOrdering.DEDUPLICATED_AND_SORTED_ASCENDING,
Block.MvOrdering.DEDUPLICATED_AND_SORTED_ASCENDING
);
// Build one operator for each field, so we get a unique map to assert on
List<Operator> operators = cases.stream()
.map(
Expand All @@ -644,8 +655,11 @@ private void testLoadAllStatus(boolean allInOnePage) {
}
}

private List<FieldCase> infoAndChecksForEachType(Block.MvOrdering docValuesMvOrdering) {
Checks checks = new Checks(docValuesMvOrdering);
private List<FieldCase> infoAndChecksForEachType(
Block.MvOrdering booleanAndNumericalDocValuesMvOrdering,
Block.MvOrdering bytesRefDocValuesMvOrdering
) {
Checks checks = new Checks(booleanAndNumericalDocValuesMvOrdering, bytesRefDocValuesMvOrdering);
List<FieldCase> r = new ArrayList<>();
r.add(new FieldCase(mapperService.fieldType(IdFieldMapper.NAME), ElementType.BYTES_REF, checks::ids, StatusChecks::id));
r.add(new FieldCase(TsidExtractingIdFieldMapper.INSTANCE.fieldType(), ElementType.BYTES_REF, checks::ids, StatusChecks::id));
Expand Down Expand Up @@ -801,7 +815,7 @@ private List<FieldCase> infoAndChecksForEachType(Block.MvOrdering docValuesMvOrd
return r;
}

record Checks(Block.MvOrdering docValuesMvOrdering) {
record Checks(Block.MvOrdering booleanAndNumericalDocValuesMvOrdering, Block.MvOrdering bytesRefDocValuesMvOrdering) {
void longs(Block block, int position, int key) {
LongVector longs = ((LongBlock) block).asVector();
assertThat(longs.getLong(position), equalTo((long) key));
Expand Down Expand Up @@ -858,7 +872,7 @@ void constantNulls(Block block, int position, int key) {
}

void mvLongsFromDocValues(Block block, int position, int key) {
mvLongs(block, position, key, docValuesMvOrdering);
mvLongs(block, position, key, booleanAndNumericalDocValuesMvOrdering);
}

void mvLongsUnordered(Block block, int position, int key) {
Expand All @@ -878,7 +892,7 @@ private void mvLongs(Block block, int position, int key, Block.MvOrdering expect
}

void mvIntsFromDocValues(Block block, int position, int key) {
mvInts(block, position, key, docValuesMvOrdering);
mvInts(block, position, key, booleanAndNumericalDocValuesMvOrdering);
}

void mvIntsUnordered(Block block, int position, int key) {
Expand All @@ -905,7 +919,7 @@ void mvShorts(Block block, int position, int key) {
assertThat(ints.getInt(offset + v), equalTo((int) (short) (2_000 * key + v)));
}
if (key % 3 > 0) {
assertThat(ints.mvOrdering(), equalTo(docValuesMvOrdering));
assertThat(ints.mvOrdering(), equalTo(booleanAndNumericalDocValuesMvOrdering));
}
}

Expand All @@ -917,7 +931,7 @@ void mvBytes(Block block, int position, int key) {
assertThat(ints.getInt(offset + v), equalTo((int) (byte) (3_000 * key + v)));
}
if (key % 3 > 0) {
assertThat(ints.mvOrdering(), equalTo(docValuesMvOrdering));
assertThat(ints.mvOrdering(), equalTo(booleanAndNumericalDocValuesMvOrdering));
}
}

Expand All @@ -928,12 +942,12 @@ void mvDoubles(Block block, int position, int key) {
assertThat(doubles.getDouble(offset + v), equalTo(key / 123_456d + v));
}
if (key % 3 > 0) {
assertThat(doubles.mvOrdering(), equalTo(docValuesMvOrdering));
assertThat(doubles.mvOrdering(), equalTo(booleanAndNumericalDocValuesMvOrdering));
}
}

void mvStringsFromDocValues(Block block, int position, int key) {
mvStrings(block, position, key, docValuesMvOrdering);
mvStrings(block, position, key, bytesRefDocValuesMvOrdering);
}

void mvStringsUnordered(Block block, int position, int key) {
Expand All @@ -960,7 +974,7 @@ void mvBools(Block block, int position, int key) {
assertThat(bools.getBoolean(offset + v), equalTo(BOOLEANS[key % 3][v]));
}
if (key % 3 > 0) {
assertThat(bools.mvOrdering(), equalTo(docValuesMvOrdering));
assertThat(bools.mvOrdering(), equalTo(booleanAndNumericalDocValuesMvOrdering));
}
}
}
Expand Down Expand Up @@ -1440,7 +1454,7 @@ private void testSequentialStoredFields(boolean sequential, int docCount) throws
0
).get(driverContext);
List<Page> results = drive(op, source.iterator(), driverContext);
Checks checks = new Checks(Block.MvOrdering.UNORDERED);
Checks checks = new Checks(Block.MvOrdering.UNORDERED, Block.MvOrdering.UNORDERED);
IntVector keys = results.get(0).<IntBlock>getBlock(1).asVector();
for (int p = 0; p < results.get(0).getPositionCount(); p++) {
int key = keys.getInt(p);
Expand All @@ -1459,7 +1473,8 @@ private void testSequentialStoredFields(boolean sequential, int docCount) throws

public void testDescriptionOfMany() throws IOException {
initIndex(1, 1);
List<FieldCase> cases = infoAndChecksForEachType(randomFrom(Block.MvOrdering.values()));
Block.MvOrdering ordering = randomFrom(Block.MvOrdering.values());
List<FieldCase> cases = infoAndChecksForEachType(ordering, ordering);

ValuesSourceReaderOperator.Factory factory = new ValuesSourceReaderOperator.Factory(
cases.stream().map(c -> c.info).toList(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.hamcrest.StringDescription;

import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.Iterator;
import java.util.List;
Expand Down Expand Up @@ -268,7 +269,8 @@ private static void dataFailure(List<DataFailure> dataFailures) {
if (f.actual instanceof List<?> a) {
actualList = a;
} else {
actualList = List.of(f.actual);
// Do not use List::of - actual can be null.
actualList = Collections.singletonList(f.actual);
}
expected.describeMismatch(actualList, description);
String prefix = "row " + f.row + " column " + f.column + ":";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@
import org.elasticsearch.xpack.esql.expression.function.UnsupportedAttribute;
import org.elasticsearch.xpack.esql.expression.function.scalar.EsqlScalarFunction;
import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.EsqlArithmeticOperation;
import org.elasticsearch.xpack.esql.plan.logical.Aggregate;
import org.elasticsearch.xpack.esql.plan.logical.Drop;
import org.elasticsearch.xpack.esql.plan.logical.Enrich;
import org.elasticsearch.xpack.esql.plan.logical.EsRelation;
import org.elasticsearch.xpack.esql.plan.logical.EsqlAggregate;
import org.elasticsearch.xpack.esql.plan.logical.EsqlUnresolvedRelation;
import org.elasticsearch.xpack.esql.plan.logical.Eval;
Expand Down Expand Up @@ -56,8 +58,6 @@
import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison;
import org.elasticsearch.xpack.ql.index.EsIndex;
import org.elasticsearch.xpack.ql.plan.TableIdentifier;
import org.elasticsearch.xpack.ql.plan.logical.Aggregate;
import org.elasticsearch.xpack.ql.plan.logical.EsRelation;
import org.elasticsearch.xpack.ql.plan.logical.Limit;
import org.elasticsearch.xpack.ql.plan.logical.LogicalPlan;
import org.elasticsearch.xpack.ql.plan.logical.Project;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.elasticsearch.xpack.esql.expression.function.UnsupportedAttribute;
import org.elasticsearch.xpack.esql.expression.function.grouping.GroupingFunction;
import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.Neg;
import org.elasticsearch.xpack.esql.plan.logical.Aggregate;
import org.elasticsearch.xpack.esql.plan.logical.Enrich;
import org.elasticsearch.xpack.esql.plan.logical.Eval;
import org.elasticsearch.xpack.esql.plan.logical.RegexExtract;
Expand All @@ -32,7 +33,6 @@
import org.elasticsearch.xpack.ql.expression.function.aggregate.AggregateFunction;
import org.elasticsearch.xpack.ql.expression.predicate.BinaryOperator;
import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison;
import org.elasticsearch.xpack.ql.plan.logical.Aggregate;
import org.elasticsearch.xpack.ql.plan.logical.Limit;
import org.elasticsearch.xpack.ql.plan.logical.LogicalPlan;
import org.elasticsearch.xpack.ql.plan.logical.OrderBy;
Expand Down
Loading

0 comments on commit f448548

Please sign in to comment.