diff --git a/docs/changelog/127146.yaml b/docs/changelog/127146.yaml new file mode 100644 index 0000000000000..a36d837f0bebd --- /dev/null +++ b/docs/changelog/127146.yaml @@ -0,0 +1,5 @@ +pr: 127146 +summary: Fix sneaky bug in single value query +area: ES|QL +type: bug +issues: [] diff --git a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/querydsl/query/SingleValueMatchQuery.java b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/querydsl/query/SingleValueMatchQuery.java index 3790890262455..91e14ea724085 100644 --- a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/querydsl/query/SingleValueMatchQuery.java +++ b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/querydsl/query/SingleValueMatchQuery.java @@ -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 { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/querydsl/query/SingleValueMathQueryTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/querydsl/query/SingleValueMathQueryTests.java index 339b07dfabc41..11a73446b6b2b 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/querydsl/query/SingleValueMathQueryTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/querydsl/query/SingleValueMathQueryTests.java @@ -48,10 +48,11 @@ interface Setup { void assertRewrite(IndexSearcher indexSearcher, Query query) throws IOException; } - @ParametersFactory + @ParametersFactory(argumentFormatting = "%s") public static List params() { List 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) }); @@ -129,13 +130,13 @@ public XContentBuilder mapping(XContentBuilder builder) throws IOException { @Override public List> build(RandomIndexWriter iw) throws IOException { - List> fieldValues = new ArrayList<>(100); + List> docs = new ArrayList<>(count); for (int i = 0; i < count; i++) { List values = values(i); - fieldValues.add(values); + docs.add(values); iw.addDocument(docFor(values)); } - return fieldValues; + return docs; } @Override @@ -154,7 +155,7 @@ private List values(int i) { int count = between(2, 10); Set set = new HashSet<>(count); while (set.size() < count) { - set.add(randomValue()); + set.add(randomValue(fieldType)); } return List.copyOf(set); } @@ -162,45 +163,66 @@ private List values(int i) { 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: + *
    + *
  • Is defined on every field
  • + *
  • Contains the same number of distinct values as documents
  • + *
+ */ + 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 docFor(Iterable values) { - List 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)); - } - } + @Override + public List> build(RandomIndexWriter iw) throws IOException { + Object first = randomValue(fieldType); + Object second = randomValue(fieldType); + List justFirst = List.of(first); + List 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 docFor(Iterable values) { + List fields = new ArrayList<>(); + for (Object v : values) { + fields.add(switch (v) { + case Double n -> new DoubleField("foo", n, Field.Store.NO); + case Float n -> new DoubleField("foo", n, Field.Store.NO); + case Number n -> new LongField("foo", n.longValue(), Field.Store.NO); + case String s -> new KeywordField("foo", s, Field.Store.NO); default -> throw new UnsupportedOperationException(); - } - return fields; + }); } + return fields; } }