From 46209591fe250ffd002325b3110bbe8b34006b50 Mon Sep 17 00:00:00 2001 From: Adrien Grand Date: Wed, 5 Dec 2018 14:06:32 +0100 Subject: [PATCH] Propagate a doc-value `format` with inner hits. (#36068) While sql passes a doc-value `format` when asking for doc-value fields, it doesn't do it when asking for fields via inner hits. --- .../querydsl/container/QueryContainer.java | 17 +++++++++---- .../xpack/sql/querydsl/query/BoolQuery.java | 6 ++--- .../xpack/sql/querydsl/query/LeafQuery.java | 2 +- .../xpack/sql/querydsl/query/NestedQuery.java | 22 ++++++++--------- .../xpack/sql/querydsl/query/NotQuery.java | 4 ++-- .../xpack/sql/querydsl/query/Query.java | 2 +- .../container/QueryContainerTests.java | 24 ++++++++++++------- .../sql/querydsl/query/BoolQueryTests.java | 10 +++++--- .../sql/querydsl/query/LeafQueryTests.java | 4 +++- .../sql/querydsl/query/NestedQueryTests.java | 24 +++++++++++-------- 10 files changed, 70 insertions(+), 45 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java index 5ef4689422b..8901ca75c8f 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java @@ -11,6 +11,7 @@ import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.search.fetch.subphase.DocValueFieldsContext; import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; import org.elasticsearch.xpack.sql.execution.search.FieldExtraction; import org.elasticsearch.xpack.sql.execution.search.SourceGenerator; @@ -29,8 +30,10 @@ import org.elasticsearch.xpack.sql.querydsl.query.MatchAll; import org.elasticsearch.xpack.sql.querydsl.query.NestedQuery; import org.elasticsearch.xpack.sql.querydsl.query.Query; import org.elasticsearch.xpack.sql.tree.Location; +import org.elasticsearch.xpack.sql.type.DataType; import java.io.IOException; +import java.util.AbstractMap; import java.util.ArrayList; import java.util.Collection; import java.util.LinkedHashMap; @@ -180,8 +183,9 @@ public class QueryContainer { List nestedRefs = new ArrayList<>(); String name = aliasName(attr); + String format = attr.field().getDataType() == DataType.DATE ? "epoch_millis" : DocValueFieldsContext.USE_DEFAULT_FORMAT; Query q = rewriteToContainNestedField(query, attr.location(), - attr.nestedParent().name(), name, attr.field().isAggregatable()); + attr.nestedParent().name(), name, format, attr.field().isAggregatable()); SearchHitFieldRef nestedFieldRef = new SearchHitFieldRef(name, attr.field().getDataType(), attr.field().isAggregatable(), attr.parent().name()); @@ -190,11 +194,13 @@ public class QueryContainer { return new Tuple<>(new QueryContainer(q, aggs, columns, aliases, pseudoFunctions, scalarFunctions, sort, limit), nestedFieldRef); } - static Query rewriteToContainNestedField(@Nullable Query query, Location location, String path, String name, boolean hasDocValues) { + static Query rewriteToContainNestedField(@Nullable Query query, Location location, String path, String name, String format, + boolean hasDocValues) { if (query == null) { /* There is no query so we must add the nested query * ourselves to fetch the field. */ - return new NestedQuery(location, path, singletonMap(name, hasDocValues), new MatchAll(location)); + return new NestedQuery(location, path, singletonMap(name, new AbstractMap.SimpleImmutableEntry<>(hasDocValues, format)), + new MatchAll(location)); } if (query.containsNestedField(path, name)) { // The query already has the nested field. Nothing to do. @@ -202,7 +208,7 @@ public class QueryContainer { } /* The query doesn't have the nested field so we have to ask * it to add it. */ - Query rewritten = query.addNestedField(path, name, hasDocValues); + Query rewritten = query.addNestedField(path, name, format, hasDocValues); if (rewritten != query) { /* It successfully added it so we can use the rewritten * query. */ @@ -210,7 +216,8 @@ public class QueryContainer { } /* There is no nested query with a matching path so we must * add the nested query ourselves just to fetch the field. */ - NestedQuery nested = new NestedQuery(location, path, singletonMap(name, hasDocValues), new MatchAll(location)); + NestedQuery nested = new NestedQuery(location, path, + singletonMap(name, new AbstractMap.SimpleImmutableEntry<>(hasDocValues, format)), new MatchAll(location)); return new BoolQuery(location, true, query, nested); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/BoolQuery.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/BoolQuery.java index 47075773b01..8b342b34c77 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/BoolQuery.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/BoolQuery.java @@ -44,9 +44,9 @@ public class BoolQuery extends Query { } @Override - public Query addNestedField(String path, String field, boolean hasDocValues) { - Query rewrittenLeft = left.addNestedField(path, field, hasDocValues); - Query rewrittenRight = right.addNestedField(path, field, hasDocValues); + public Query addNestedField(String path, String field, String format, boolean hasDocValues) { + Query rewrittenLeft = left.addNestedField(path, field, format, hasDocValues); + Query rewrittenRight = right.addNestedField(path, field, format, hasDocValues); if (rewrittenLeft == left && rewrittenRight == right) { return this; } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/LeafQuery.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/LeafQuery.java index c66de04f550..4c0a5b4dcd6 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/LeafQuery.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/LeafQuery.java @@ -20,7 +20,7 @@ abstract class LeafQuery extends Query { } @Override - public Query addNestedField(String path, String field, boolean hasDocValues) { + public Query addNestedField(String path, String field, String format, boolean hasDocValues) { // No leaf queries are nested return this; } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/NestedQuery.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/NestedQuery.java index 03af1433f30..4224c2adc8a 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/NestedQuery.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/NestedQuery.java @@ -5,11 +5,11 @@ */ package org.elasticsearch.xpack.sql.querydsl.query; +import java.util.AbstractMap; import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Map.Entry; import java.util.Objects; import org.apache.lucene.search.join.ScoreMode; @@ -37,14 +37,14 @@ public class NestedQuery extends Query { private static final List NO_STORED_FIELD = singletonList(StoredFieldsContext._NONE_); private final String path; - private final Map fields; + private final Map> fields; // field -> (useDocValues, format) private final Query child; public NestedQuery(Location location, String path, Query child) { this(location, path, emptyMap(), child); } - public NestedQuery(Location location, String path, Map fields, Query child) { + public NestedQuery(Location location, String path, Map> fields, Query child) { super(location); if (path == null) { throw new IllegalArgumentException("path is required"); @@ -68,10 +68,10 @@ public class NestedQuery extends Query { } @Override - public Query addNestedField(String path, String field, boolean hasDocValues) { + public Query addNestedField(String path, String field, String format, boolean hasDocValues) { if (false == this.path.equals(path)) { // I'm not at the right path so let my child query have a crack at it - Query rewrittenChild = child.addNestedField(path, field, hasDocValues); + Query rewrittenChild = child.addNestedField(path, field, format, hasDocValues); if (rewrittenChild == child) { return this; } @@ -81,9 +81,9 @@ public class NestedQuery extends Query { // I already have the field, no rewriting needed return this; } - Map newFields = new HashMap<>(fields.size() + 1); + Map> newFields = new HashMap<>(fields.size() + 1); newFields.putAll(fields); - newFields.put(field, hasDocValues); + newFields.put(field, new AbstractMap.SimpleImmutableEntry<>(hasDocValues, format)); return new NestedQuery(location(), path, unmodifiableMap(newFields), child); } @@ -113,9 +113,9 @@ public class NestedQuery extends Query { boolean noSourceNeeded = true; List sourceFields = new ArrayList<>(); - for (Entry entry : fields.entrySet()) { - if (entry.getValue()) { - ihb.addDocValueField(entry.getKey()); + for (Map.Entry> entry : fields.entrySet()) { + if (entry.getValue().getKey()) { + ihb.addDocValueField(entry.getKey(), entry.getValue().getValue()); } else { sourceFields.add(entry.getKey()); @@ -141,7 +141,7 @@ public class NestedQuery extends Query { return path; } - Map fields() { + Map> fields() { return fields; } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/NotQuery.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/NotQuery.java index d6464740c85..d44e62c230f 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/NotQuery.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/NotQuery.java @@ -34,8 +34,8 @@ public class NotQuery extends Query { } @Override - public Query addNestedField(String path, String field, boolean hasDocValues) { - Query rewrittenChild = child.addNestedField(path, field, hasDocValues); + public Query addNestedField(String path, String field, String format, boolean hasDocValues) { + Query rewrittenChild = child.addNestedField(path, field, format, hasDocValues); if (child == rewrittenChild) { return this; } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/Query.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/Query.java index 057454b6fd8..7691ef42041 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/Query.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/Query.java @@ -44,7 +44,7 @@ public abstract class Query { * @return a new query if we could add the nested field, the same query * instance otherwise */ - public abstract Query addNestedField(String path, String field, boolean hasDocValues); + public abstract Query addNestedField(String path, String field, String format, boolean hasDocValues); /** * Attach the one and only one matching nested query's filter to this diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainerTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainerTests.java index f943e325cf7..84cb62a5ada 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainerTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainerTests.java @@ -5,6 +5,9 @@ */ package org.elasticsearch.xpack.sql.querydsl.container; +import java.util.AbstractMap.SimpleImmutableEntry; + +import org.elasticsearch.search.fetch.subphase.DocValueFieldsContext; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.sql.querydsl.query.BoolQuery; import org.elasticsearch.xpack.sql.querydsl.query.MatchAll; @@ -21,18 +24,21 @@ public class QueryContainerTests extends ESTestCase { private Location location = LocationTests.randomLocation(); private String path = randomAlphaOfLength(5); private String name = randomAlphaOfLength(5); + private String format = DocValueFieldsContext.USE_DEFAULT_FORMAT; private boolean hasDocValues = randomBoolean(); public void testRewriteToContainNestedFieldNoQuery() { - Query expected = new NestedQuery(location, path, singletonMap(name, hasDocValues), new MatchAll(location)); - assertEquals(expected, QueryContainer.rewriteToContainNestedField(null, location, path, name, hasDocValues)); + Query expected = new NestedQuery(location, path, singletonMap(name, new SimpleImmutableEntry<>(hasDocValues, format)), + new MatchAll(location)); + assertEquals(expected, QueryContainer.rewriteToContainNestedField(null, location, path, name, format, hasDocValues)); } public void testRewriteToContainsNestedFieldWhenContainsNestedField() { Query original = new BoolQuery(location, true, - new NestedQuery(location, path, singletonMap(name, hasDocValues), new MatchAll(location)), + new NestedQuery(location, path, singletonMap(name, new SimpleImmutableEntry<>(hasDocValues, format)), + new MatchAll(location)), new RangeQuery(location, randomAlphaOfLength(5), 0, randomBoolean(), 100, randomBoolean())); - assertSame(original, QueryContainer.rewriteToContainNestedField(original, location, path, name, randomBoolean())); + assertSame(original, QueryContainer.rewriteToContainNestedField(original, location, path, name, format, randomBoolean())); } public void testRewriteToContainsNestedFieldWhenCanAddNestedField() { @@ -41,16 +47,18 @@ public class QueryContainerTests extends ESTestCase { new NestedQuery(location, path, emptyMap(), new MatchAll(location)), buddy); Query expected = new BoolQuery(location, true, - new NestedQuery(location, path, singletonMap(name, hasDocValues), new MatchAll(location)), + new NestedQuery(location, path, singletonMap(name, new SimpleImmutableEntry<>(hasDocValues, format)), + new MatchAll(location)), buddy); - assertEquals(expected, QueryContainer.rewriteToContainNestedField(original, location, path, name, hasDocValues)); + assertEquals(expected, QueryContainer.rewriteToContainNestedField(original, location, path, name, format, hasDocValues)); } public void testRewriteToContainsNestedFieldWhenDoesNotContainNestedFieldAndCantAdd() { Query original = new RangeQuery(location, randomAlphaOfLength(5), 0, randomBoolean(), 100, randomBoolean()); Query expected = new BoolQuery(location, true, original, - new NestedQuery(location, path, singletonMap(name, hasDocValues), new MatchAll(location))); - assertEquals(expected, QueryContainer.rewriteToContainNestedField(original, location, path, name, hasDocValues)); + new NestedQuery(location, path, singletonMap(name, new SimpleImmutableEntry<>(hasDocValues, format)), + new MatchAll(location))); + assertEquals(expected, QueryContainer.rewriteToContainNestedField(original, location, path, name, format, hasDocValues)); } } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/query/BoolQueryTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/query/BoolQueryTests.java index 76c582f236b..5837b069654 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/query/BoolQueryTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/query/BoolQueryTests.java @@ -5,6 +5,7 @@ */ package org.elasticsearch.xpack.sql.querydsl.query; +import org.elasticsearch.search.fetch.subphase.DocValueFieldsContext; import org.elasticsearch.search.sort.NestedSortBuilder; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.sql.tree.Location; @@ -12,6 +13,7 @@ import org.elasticsearch.xpack.sql.tree.LocationTests; import java.util.Arrays; import java.util.List; +import java.util.AbstractMap.SimpleImmutableEntry; import java.util.function.Function; import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode; @@ -50,14 +52,15 @@ public class BoolQueryTests extends ESTestCase { public void testAddNestedField() { Query q = boolQueryWithoutNestedChildren(); - assertSame(q, q.addNestedField(randomAlphaOfLength(5), randomAlphaOfLength(5), randomBoolean())); + assertSame(q, q.addNestedField(randomAlphaOfLength(5), randomAlphaOfLength(5), DocValueFieldsContext.USE_DEFAULT_FORMAT, + randomBoolean())); String path = randomAlphaOfLength(5); String field = randomAlphaOfLength(5); q = boolQueryWithNestedChildren(path, field); String newField = randomAlphaOfLength(5); boolean hasDocValues = randomBoolean(); - Query rewritten = q.addNestedField(path, newField, hasDocValues); + Query rewritten = q.addNestedField(path, newField, DocValueFieldsContext.USE_DEFAULT_FORMAT, hasDocValues); assertNotSame(q, rewritten); assertTrue(rewritten.containsNestedField(path, newField)); } @@ -83,7 +86,8 @@ public class BoolQueryTests extends ESTestCase { private Query boolQueryWithNestedChildren(String path, String field) { NestedQuery match = new NestedQuery(LocationTests.randomLocation(), path, - singletonMap(field, randomBoolean()), new MatchAll(LocationTests.randomLocation())); + singletonMap(field, new SimpleImmutableEntry<>(randomBoolean(), DocValueFieldsContext.USE_DEFAULT_FORMAT)), + new MatchAll(LocationTests.randomLocation())); Query matchAll = new MatchAll(LocationTests.randomLocation()); Query left; Query right; diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/query/LeafQueryTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/query/LeafQueryTests.java index 232040b79bc..52043003ab5 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/query/LeafQueryTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/query/LeafQueryTests.java @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.sql.querydsl.query; import org.elasticsearch.index.query.QueryBuilder; +import org.elasticsearch.search.fetch.subphase.DocValueFieldsContext; import org.elasticsearch.search.sort.NestedSortBuilder; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.sql.tree.Location; @@ -52,7 +53,8 @@ public class LeafQueryTests extends ESTestCase { public void testAddNestedField() { Query query = new DummyLeafQuery(LocationTests.randomLocation()); // Leaf queries don't contain nested fields. - assertSame(query, query.addNestedField(randomAlphaOfLength(5), randomAlphaOfLength(5), randomBoolean())); + assertSame(query, query.addNestedField(randomAlphaOfLength(5), randomAlphaOfLength(5), DocValueFieldsContext.USE_DEFAULT_FORMAT, + randomBoolean())); } public void testEnrichNestedSort() { diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/query/NestedQueryTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/query/NestedQueryTests.java index 7cd53bc73de..3e228954949 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/query/NestedQueryTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/query/NestedQueryTests.java @@ -5,12 +5,14 @@ */ package org.elasticsearch.xpack.sql.querydsl.query; +import org.elasticsearch.search.fetch.subphase.DocValueFieldsContext; import org.elasticsearch.search.sort.NestedSortBuilder; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; import org.elasticsearch.xpack.sql.tree.Location; import org.elasticsearch.xpack.sql.tree.LocationTests; +import java.util.AbstractMap.SimpleImmutableEntry; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; @@ -38,11 +40,11 @@ public class NestedQueryTests extends ESTestCase { return new NestedQuery(LocationTests.randomLocation(), randomAlphaOfLength(5), randomFields(), randomQuery(depth)); } - private static Map randomFields() { + private static Map> randomFields() { int size = between(0, 5); - Map fields = new HashMap<>(size); + Map> fields = new HashMap<>(size); while (fields.size() < size) { - fields.put(randomAlphaOfLength(5), randomBoolean()); + fields.put(randomAlphaOfLength(5), new SimpleImmutableEntry<>(randomBoolean(), DocValueFieldsContext.USE_DEFAULT_FORMAT)); } return fields; } @@ -77,20 +79,20 @@ public class NestedQueryTests extends ESTestCase { NestedQuery q = randomNestedQuery(0); for (String field : q.fields().keySet()) { // add does nothing if the field is already there - assertSame(q, q.addNestedField(q.path(), field, randomBoolean())); + assertSame(q, q.addNestedField(q.path(), field, DocValueFieldsContext.USE_DEFAULT_FORMAT, randomBoolean())); String otherPath = randomValueOtherThan(q.path(), () -> randomAlphaOfLength(5)); // add does nothing if the path doesn't match - assertSame(q, q.addNestedField(otherPath, randomAlphaOfLength(5), randomBoolean())); + assertSame(q, q.addNestedField(otherPath, randomAlphaOfLength(5), DocValueFieldsContext.USE_DEFAULT_FORMAT, randomBoolean())); } // if the field isn't in the list then add rewrites to a query with all the old fields and the new one String newField = randomValueOtherThanMany(q.fields()::containsKey, () -> randomAlphaOfLength(5)); boolean hasDocValues = randomBoolean(); - NestedQuery added = (NestedQuery) q.addNestedField(q.path(), newField, hasDocValues); + NestedQuery added = (NestedQuery) q.addNestedField(q.path(), newField, DocValueFieldsContext.USE_DEFAULT_FORMAT, hasDocValues); assertNotSame(q, added); - assertThat(added.fields(), hasEntry(newField, hasDocValues)); + assertThat(added.fields(), hasEntry(newField, new SimpleImmutableEntry<>(hasDocValues, DocValueFieldsContext.USE_DEFAULT_FORMAT))); assertTrue(added.containsNestedField(q.path(), newField)); - for (Map.Entry field : q.fields().entrySet()) { + for (Map.Entry> field : q.fields().entrySet()) { assertThat(added.fields(), hasEntry(field.getKey(), field.getValue())); assertTrue(added.containsNestedField(q.path(), field.getKey())); } @@ -129,7 +131,9 @@ public class NestedQueryTests extends ESTestCase { } public void testToString() { - NestedQuery q = new NestedQuery(new Location(1, 1), "a.b", singletonMap("f", true), new MatchAll(new Location(1, 1))); - assertEquals("NestedQuery@1:2[a.b.{f=true}[MatchAll@1:2[]]]", q.toString()); + NestedQuery q = new NestedQuery(new Location(1, 1), "a.b", + singletonMap("f", new SimpleImmutableEntry<>(true, DocValueFieldsContext.USE_DEFAULT_FORMAT)), + new MatchAll(new Location(1, 1))); + assertEquals("NestedQuery@1:2[a.b.{f=true=use_field_mapping}[MatchAll@1:2[]]]", q.toString()); } }