Skip to content

ESQL: Remove the NESTED DataType #111495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nik9000 merged 11 commits into from
Aug 6, 2024
Merged

ESQL: Remove the NESTED DataType #111495

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
581ac34
ESQL: Remove the `NESTED` DataType
nik9000 Jul 31, 2024
d0405b2
More tests for nested
nik9000 Aug 1, 2024
ff5b756
Merge branch 'main' into esql_drop_nested
nik9000 Aug 1, 2024
4a94155
Maybe this
nik9000 Aug 1, 2024
2fa37c1
Test
nik9000 Aug 1, 2024
890be2d
Merge branch 'main' into esql_drop_nested
nik9000 Aug 5, 2024
47dac60
more tests
nik9000 Aug 5, 2024
deef905
Merge branch 'main' into esql_drop_nested
nik9000 Aug 6, 2024
98c316d
Transport change.....
nik9000 Aug 6, 2024
25460e9
Explain the read more
nik9000 Aug 6, 2024
5a6cbbd
Merge branch 'main' into esql_drop_nested
nik9000 Aug 6, 2024
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
4 changes: 1 addition & 3 deletions x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ public enum DataType {
// 8.15.2-SNAPSHOT is 15 bytes, most are shorter, some can be longer
VERSION(builder().esType("version").estimatedSize(15).docValues()),
OBJECT(builder().esType("object").unknownSize()),
NESTED(builder().esType("nested").unknownSize()),
SOURCE(builder().esType(SourceFieldMapper.NAME).unknownSize()),
DATE_PERIOD(builder().typeName("DATE_PERIOD").estimatedSize(3 * Integer.BYTES)),
TIME_DURATION(builder().typeName("TIME_DURATION").estimatedSize(Integer.BYTES + Long.BYTES)),
Expand Down Expand Up @@ -287,7 +286,7 @@ public static boolean isPrimitiveAndSupported(DataType t) {
}

public static boolean isPrimitive(DataType t) {
return t != OBJECT && t != NESTED;
return t != OBJECT;
}

public static boolean isNull(DataType t) {
Expand Down Expand Up @@ -335,7 +334,6 @@ public static boolean areCompatible(DataType left, DataType right) {
*/
public static boolean isRepresentable(DataType t) {
return t != OBJECT
&& t != NESTED
&& t != UNSUPPORTED
&& t != DATE_PERIOD
&& t != TIME_DURATION
Expand Down
7 changes: 5 additions & 2 deletions ...k/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/LoadMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import static java.util.Collections.emptyMap;
import static org.elasticsearch.xpack.esql.core.type.DataType.DATETIME;
import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
import static org.elasticsearch.xpack.esql.core.type.DataType.NESTED;
import static org.elasticsearch.xpack.esql.core.type.DataType.OBJECT;
import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT;
import static org.elasticsearch.xpack.esql.core.type.DataType.UNSUPPORTED;
Expand Down Expand Up @@ -79,10 +78,14 @@ private static void walkMapping(String name, Object value, Map<String, EsField>
if (value instanceof Map) {
Map<String, Object> content = (Map<String, Object>) value;

if ("nested".equals(content.get("type"))) {
// Nested fields are entirely removed by IndexResolver so we mimic it.
return;
}
// extract field type
DataType esDataType = getType(content);
final Map<String, EsField> properties;
if (esDataType == OBJECT || esDataType == NESTED) {
if (esDataType == OBJECT) {

@nik9000 nik9000 Jul 31, 2024

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.

This is from Types in QL, but it's only used in testing in ESQL. I think this safe for now.

properties = fromEs(content);
} else if (content.containsKey("fields")) {
// Check for multifields
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/PositionToXContent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ protected XContentBuilder valueToXContent(XContentBuilder builder, ToXContent.Pa
}
}
};
case DATE_PERIOD, TIME_DURATION, DOC_DATA_TYPE, TSID_DATA_TYPE, SHORT, BYTE, OBJECT, NESTED, FLOAT, HALF_FLOAT, SCALED_FLOAT,
case DATE_PERIOD, TIME_DURATION, DOC_DATA_TYPE, TSID_DATA_TYPE, SHORT, BYTE, OBJECT, FLOAT, HALF_FLOAT, SCALED_FLOAT,
PARTIAL_AGG -> throw new IllegalArgumentException("can't convert values of type [" + columnInfo.type() + "]");
};
}
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/ResponseValueUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,8 @@ private static Object valueAt(DataType dataType, Block block, int offset, BytesR
throw new UncheckedIOException(e);
}
}
case SHORT, BYTE, FLOAT, HALF_FLOAT, SCALED_FLOAT, OBJECT, NESTED, DATE_PERIOD, TIME_DURATION, DOC_DATA_TYPE, TSID_DATA_TYPE,
NULL, PARTIAL_AGG -> throw EsqlIllegalArgumentException.illegalDataType(dataType);
case SHORT, BYTE, FLOAT, HALF_FLOAT, SCALED_FLOAT, OBJECT, DATE_PERIOD, TIME_DURATION, DOC_DATA_TYPE, TSID_DATA_TYPE, NULL,
PARTIAL_AGG -> throw EsqlIllegalArgumentException.illegalDataType(dataType);
};
}
}
5 changes: 2 additions & 3 deletions x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,6 @@
import static org.elasticsearch.xpack.esql.core.type.DataType.IP;
import static org.elasticsearch.xpack.esql.core.type.DataType.KEYWORD;
import static org.elasticsearch.xpack.esql.core.type.DataType.LONG;
import static org.elasticsearch.xpack.esql.core.type.DataType.NESTED;
import static org.elasticsearch.xpack.esql.core.type.DataType.TEXT;
import static org.elasticsearch.xpack.esql.core.type.DataType.VERSION;
import static org.elasticsearch.xpack.esql.core.type.DataType.isTemporalAmount;
Expand Down Expand Up @@ -245,8 +244,8 @@ private static void mappingAsAttributes(List<Attribute> list, Source source, Fie
if (DataType.isPrimitive(type)) {
list.add(attribute);
}
// allow compound object even if they are unknown (but not NESTED)
if (type != NESTED && fieldProperties.isEmpty() == false) {
// allow compound object even if they are unknown
if (fieldProperties.isEmpty() == false) {

@nik9000 nik9000 Jul 31, 2024

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.

This is the most interesting line, I think. But we don't make NESTED any more so I feel like this is pretty safe.

@nik9000 nik9000 Aug 1, 2024

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.

So this would change things. It'd make the sub-fields of the NESTED object appear. I think. I'll double check.

@astefan astefan Aug 2, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, I think this is ok. Stuff like object with sub-fields still work 👍 ; I tested an empty index with this kind of mapping:

            "name": {
                "type": "object",
                "properties": {
                    "first_name": {
                        "type": "keyword"
                    },
                    "last_name": {
                        "type": "keyword"
                    }
                }
            }

and we get back only name.first_name and name.last_name. Which I think it's ok? Now I'm wondering if OBJECT has the same treatment as NESTED, since it disappears.

Also, nested doesn't seem to accept fields, but what it does (and I forgot about it) is include_in_parent but I don't think we ever supported this since _field_caps behaves identically with include_in_parent true or false.

mappingAsAttributes(list, source, attribute, fieldProperties);
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ private PhysicalOperation planTopN(TopNExec topNExec, LocalExecutionPlannerConte
case TEXT, KEYWORD -> TopNEncoder.UTF8;
case VERSION -> TopNEncoder.VERSION;
case BOOLEAN, NULL, BYTE, SHORT, INTEGER, LONG, DOUBLE, FLOAT, HALF_FLOAT, DATETIME, DATE_PERIOD, TIME_DURATION, OBJECT,
NESTED, SCALED_FLOAT, UNSIGNED_LONG, DOC_DATA_TYPE, TSID_DATA_TYPE -> TopNEncoder.DEFAULT_SORTABLE;
SCALED_FLOAT, UNSIGNED_LONG, DOC_DATA_TYPE, TSID_DATA_TYPE -> TopNEncoder.DEFAULT_SORTABLE;
case GEO_POINT, CARTESIAN_POINT, GEO_SHAPE, CARTESIAN_SHAPE, COUNTER_LONG, COUNTER_INTEGER, COUNTER_DOUBLE ->
TopNEncoder.DEFAULT_UNSORTABLE;
// unsupported fields are encoded as BytesRef, we'll use the same encoder; all values should be null at this point
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/PlannerUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,8 @@ public static ElementType toElementType(DataType dataType, MappedFieldType.Field
case GEO_POINT, CARTESIAN_POINT -> fieldExtractPreference == DOC_VALUES ? ElementType.LONG : ElementType.BYTES_REF;
case GEO_SHAPE, CARTESIAN_SHAPE -> ElementType.BYTES_REF;
case PARTIAL_AGG -> ElementType.COMPOSITE;
case SHORT, BYTE, DATE_PERIOD, TIME_DURATION, OBJECT, NESTED, FLOAT, HALF_FLOAT, SCALED_FLOAT ->
throw EsqlIllegalArgumentException.illegalDataType(dataType);
case SHORT, BYTE, DATE_PERIOD, TIME_DURATION, OBJECT, FLOAT, HALF_FLOAT, SCALED_FLOAT -> throw EsqlIllegalArgumentException
.illegalDataType(dataType);
};
}

Expand Down
25 changes: 23 additions & 2 deletions x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/IndexResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ public void resolveAsMergedMapping(String indexWildcard, Set<String> fieldNames,
);
}

// public for testing only
public IndexResolution mergedMappings(String indexPattern, FieldCapabilitiesResponse fieldCapsResponse) {
assert ThreadPool.assertCurrentThreadPool(ThreadPool.Names.SEARCH_COORDINATION); // too expensive to run this on a transport worker
if (fieldCapsResponse.getIndexResponses().isEmpty()) {
Expand All @@ -93,8 +94,9 @@ public IndexResolution mergedMappings(String indexPattern, FieldCapabilitiesResp
// TODO flattened is simpler - could we get away with that?
String[] names = fieldsCaps.keySet().toArray(new String[0]);
Arrays.sort(names);
Set<String> forbiddenFields = new HashSet<>();
Map<String, EsField> rootFields = new HashMap<>();
for (String name : names) {
name: for (String name : names) {
Map<String, EsField> fields = rootFields;
String fullName = name;
boolean isAlias = false;
Expand All @@ -105,6 +107,9 @@ public IndexResolution mergedMappings(String indexPattern, FieldCapabilitiesResp
break;
}
String parent = name.substring(0, nextDot);
if (forbiddenFields.contains(parent)) {
continue name;
}
EsField obj = fields.get(parent);
if (obj == null) {
obj = new EsField(parent, OBJECT, new HashMap<>(), false, true);
Expand All @@ -116,9 +121,16 @@ public IndexResolution mergedMappings(String indexPattern, FieldCapabilitiesResp
fields = obj.getProperties();
name = name.substring(nextDot + 1);
}

List<IndexFieldCapabilities> caps = fieldsCaps.get(fullName);
if (allNested(caps)) {
forbiddenFields.add(name);
continue;
}
// TODO we're careful to make isAlias match IndexResolver - but do we use it?

EsField field = firstUnsupportedParent == null
? createField(fieldCapsResponse, name, fullName, fieldsCaps.get(fullName), isAlias)
? createField(fieldCapsResponse, name, fullName, caps, isAlias)
: new UnsupportedEsField(
fullName,
firstUnsupportedParent.getOriginalType(),
Expand All @@ -144,6 +156,15 @@ public IndexResolution mergedMappings(String indexPattern, FieldCapabilitiesResp
return IndexResolution.valid(new EsIndex(indexPattern, rootFields, concreteIndices));
}

private boolean allNested(List<IndexFieldCapabilities> caps) {
for (IndexFieldCapabilities cap : caps) {
if (false == cap.type().equalsIgnoreCase("nested")) {
return false;
}
}
return true;
}

private static Map<String, List<IndexFieldCapabilities>> collectFieldCaps(FieldCapabilitiesResponse fieldCapsResponse) {
Set<String> seenHashes = new HashSet<>();
Map<String, List<IndexFieldCapabilities>> fieldsCaps = new HashMap<>();
Expand Down
4 changes: 2 additions & 2 deletions .../test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ public static Literal randomLiteral(DataType type) {
throw new UncheckedIOException(e);
}
}
case UNSUPPORTED, OBJECT, NESTED, DOC_DATA_TYPE, TSID_DATA_TYPE, PARTIAL_AGG -> throw new IllegalArgumentException(
case UNSUPPORTED, OBJECT, DOC_DATA_TYPE, TSID_DATA_TYPE, PARTIAL_AGG -> throw new IllegalArgumentException(
"can't make random values for [" + type.typeName() + "]"
);
}, type);
Expand Down Expand Up @@ -481,7 +481,7 @@ public static Stream<DataType> validFunctionParameters() {
*/
return false;
}
if (t == DataType.OBJECT || t == DataType.NESTED) {
if (t == DataType.OBJECT) {
// Object and nested fields aren't supported by any functions yet
return false;
}
Expand Down
30 changes: 30 additions & 0 deletions x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_unsupported_types.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,3 +390,33 @@ unsupported with sort:
- match: { values.0.26: xy }
- match: { values.0.27: "foo bar" }
- match: { values.0.28: 3 }

---
nested:
- do:
bulk:
index: test
refresh: true
body:
- { "index": { } }
- {
"find_me": 1,
"nested": {
"foo": 1,
"bar": "bar",
"baz": 1.9
}
}

- do:
allowed_warnings_regex:
- "Field \\[.*\\] cannot be retrieved, it is unsupported or not indexed; returning null"
- "No limit defined, adding default limit of \\[.*\\]"
esql.query:
body:
query: 'FROM test | WHERE find_me == 1 | KEEP n*'

# The `nested` field is not visible, nor are any of it's subfields.
- match: { columns.0.name: name }
- match: { columns.0.type: keyword }
- match: { values: [[null]] }