Skip to content

[ML][Inference] fix support for nested fields #50258

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.

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

Conversation

benwtrent
Copy link
Member

@benwtrent benwtrent commented Dec 17, 2019

This fixes support for nested fields

We now support fully nested, fully collapsed, or a mix of both on inference docs.

ES mappings allow the _source to be any combination of nested objects + dot delimited fields.
So, we should do our best to find the best path down the Map to
Examples:

    // search for "a.b.c.d" in the following maps should return 2
    [ {
         "a.b.c.d" : 2 // "fully collapsed"
     },
     {
         "a" :{"b": {"c": {"d" : 2}}} // "fully nested"
     },
     { 
         "a": {"b.c": {"d": 2}} // arbitrary mix
     }]

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM, left a suggestion about the java doc it would nice to improve.

@@ -128,7 +129,7 @@ void handleResponse(InternalInferModelAction.Response response,
Map<String, Object> fields = new HashMap<>(ingestDocument.getSourceAndMetadata());
if (fieldMapping != null) {
fieldMapping.forEach((src, dest) -> {
Object srcValue = fields.remove(src);
Copy link
Member

Choose a reason for hiding this comment

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

This is not the same as src is left in the fields map. I don't think it matters in terms of the logic only that a larger map than is necessary it being passed around.

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidkyle I suppose. This would require the dig method to have a sister "remove" method. For simplicity I opted not to include it. Having a larger map passed around is pretty cheap right now. We may have issues ones we support more than a ~100 features.

* "a.b.c": {"d": 2}
* }
*
* To exhaustively explore all these paths would result in 2^n-1 total possible paths, where {@code n = path.split("\\.").length}
Copy link
Member

Choose a reason for hiding this comment

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

By possible paths you mean ways of constructing the equivalent path a.b.c.d where each element could be a nested object or a string containing '.'. And the 2^n-1 comes from the observation that each separator in the path could either be a '.' or a nested object (':') i.e. one of two choices and there are n-1 separators.

Could you make the comment a bit clearer please.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can

…m:benwtrent/elasticsearch into feature/ml-inference-handle-nested-fields
Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM

@benwtrent
Copy link
Member Author

run elasticsearch-ci/2

@benwtrent
Copy link
Member Author

@elasticmachine update branch

@benwtrent benwtrent requested a review from davidkyle December 18, 2019 15:06
Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM nice work 👍

@benwtrent benwtrent merged commit e9e6a4a into elastic:master Dec 18, 2019
@benwtrent benwtrent deleted the feature/ml-inference-handle-nested-fields branch December 18, 2019 19:03
benwtrent added a commit to benwtrent/elasticsearch that referenced this pull request Dec 18, 2019
This fixes support for nested fields

We now support fully nested, fully collapsed, or a mix of both on inference docs.

ES mappings allow the `_source` to be any combination of nested objects + dot delimited fields.
So, we should do our best to find the best path down the Map for the desired field.
benwtrent added a commit that referenced this pull request Dec 18, 2019
This fixes support for nested fields

We now support fully nested, fully collapsed, or a mix of both on inference docs.

ES mappings allow the `_source` to be any combination of nested objects + dot delimited fields.
So, we should do our best to find the best path down the Map for the desired field.
SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020
This fixes support for nested fields

We now support fully nested, fully collapsed, or a mix of both on inference docs. 

ES mappings allow the `_source` to be any combination of nested objects + dot delimited fields. 
So, we should do our best to find the best path down the Map for the desired field.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants