Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
5 changes: 5 additions & 0 deletions docs/changelog/127146.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 127146
summary: Fix sneaky bug in single value query
area: ES|QL
type: bug
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
}
} else if (lfd instanceof LeafOrdinalsFieldData) {
final Terms terms = context.reader().terms(fieldData.getFieldName());
if (terms == null || terms.getDocCount() != context.reader().maxDoc() || terms.size() != terms.getDocCount()) {
if (terms == null || terms.getDocCount() != context.reader().maxDoc() || terms.getSumDocFreq() != terms.getDocCount()) {
return super.rewrite(indexSearcher);
}
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,11 @@ interface Setup {
void assertRewrite(IndexSearcher indexSearcher, Query query) throws IOException;
}

@ParametersFactory
@ParametersFactory(argumentFormatting = "%s")
public static List<Object[]> params() {
List<Object[]> params = new ArrayList<>();
for (String fieldType : new String[] { "long", "integer", "short", "byte", "double", "float", "keyword" }) {
params.add(new Object[] { new SneakyTwo(fieldType) });
for (boolean multivaluedField : new boolean[] { true, false }) {
for (boolean allowEmpty : new boolean[] { true, false }) {
params.add(new Object[] { new StandardSetup(fieldType, multivaluedField, allowEmpty, 100) });
Expand Down Expand Up @@ -129,13 +130,13 @@ public XContentBuilder mapping(XContentBuilder builder) throws IOException {

@Override
public List<List<Object>> build(RandomIndexWriter iw) throws IOException {
List<List<Object>> fieldValues = new ArrayList<>(100);
List<List<Object>> docs = new ArrayList<>(count);
for (int i = 0; i < count; i++) {
List<Object> values = values(i);
fieldValues.add(values);
docs.add(values);
iw.addDocument(docFor(values));
}
return fieldValues;
return docs;
}

@Override
Expand All @@ -154,53 +155,79 @@ private List<Object> values(int i) {
int count = between(2, 10);
Set<Object> set = new HashSet<>(count);
while (set.size() < count) {
set.add(randomValue());
set.add(randomValue(fieldType));
}
return List.copyOf(set);
}
// i == 0 forces at least one empty field when we're configured for empty fields
if (empty && (i == 0 || randomBoolean())) {
return List.of();
}
return List.of(randomValue());
return List.of(randomValue(fieldType));
}
}

private Object randomValue() {
return switch (fieldType) {
case "long" -> randomLong();
case "integer" -> randomInt();
case "short" -> randomShort();
case "byte" -> randomByte();
case "double" -> randomDouble();
case "float" -> randomFloat();
case "keyword" -> randomAlphaOfLength(5);
default -> throw new UnsupportedOperationException();
};
/**
* Tests a scenario where we were incorrectly rewriting {@code keyword} fields to
* {@link MatchAllDocsQuery} when:
* <ul>
* <li>Is defined on every field</li>
* <li>Contains the same number of distinct values as documents</li>
* </ul>
*/
private record SneakyTwo(String fieldType) implements Setup {
@Override
public XContentBuilder mapping(XContentBuilder builder) throws IOException {
return builder.startObject("foo").field("type", fieldType).endObject();
}

private List<IndexableField> docFor(Iterable<Object> values) {
List<IndexableField> fields = new ArrayList<>();
switch (fieldType) {
case "long", "integer", "short", "byte" -> {
for (Object v : values) {
long l = ((Number) v).longValue();
fields.add(new LongField("foo", l, Field.Store.NO));
}
}
case "double", "float" -> {
for (Object v : values) {
double d = ((Number) v).doubleValue();
fields.add(new DoubleField("foo", d, Field.Store.NO));
}
}
case "keyword" -> {
for (Object v : values) {
fields.add(new KeywordField("foo", v.toString(), Field.Store.NO));
}
}
default -> throw new UnsupportedOperationException();
@Override
public List<List<Object>> build(RandomIndexWriter iw) throws IOException {
Object first = randomValue(fieldType);
Object second = randomValue(fieldType);
List<Object> justFirst = List.of(first);
List<Object> both = List.of(first, second);
iw.addDocument(docFor(justFirst));
iw.addDocument(docFor(both));
return List.of(justFirst, both);
}

@Override
public void assertRewrite(IndexSearcher indexSearcher, Query query) throws IOException {
// There are multivalued fields
assertThat(query.rewrite(indexSearcher), sameInstance(query));
}
}

private static Object randomValue(String fieldType) {
return switch (fieldType) {
case "long" -> randomLong();
case "integer" -> randomInt();
case "short" -> randomShort();
case "byte" -> randomByte();
case "double" -> randomDouble();
case "float" -> randomFloat();
case "keyword" -> randomAlphaOfLength(5);
default -> throw new UnsupportedOperationException();
};
}

private static List<IndexableField> docFor(Iterable<Object> values) {
List<IndexableField> fields = new ArrayList<>();
for (Object v : values) {
if (v instanceof Double n) {
fields.add(new DoubleField("foo", n, Field.Store.NO));
} else if (v instanceof Float n) {
fields.add(new DoubleField("foo", n, Field.Store.NO));
} else if (v instanceof Number n) {
long l = n.longValue();
fields.add(new LongField("foo", l, Field.Store.NO));
} else if (v instanceof String s) {
fields.add(new KeywordField("foo", v.toString(), Field.Store.NO));
} else {
throw new UnsupportedOperationException();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: could be simplified with switch like in #120875

return fields;
}
return fields;
}
}
Loading