Skip to content
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

[ML][Data Frame] Add deduced mappings to _preview response payload #43742

Merged

Conversation

benwtrent
Copy link
Member

This updates the _preview response payload to include the deduced mappings. The "mapping" format returned from Pivot#deduceMappings has a non-standard format of {"field1": "field1Type", "field2.subfield": "field2.subfieldType"}. So, I opted to transform it so that it is in an appropriate format of

"properties": {
   "field1": {
       "type": "field1Type"
   },
   "field2.subfield": {
       "type": "field2.subfieldType"
   }
}

Related to: elastic/kibana#39250

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

Copy link

@hendrikmuhs hendrikmuhs left a comment

Choose a reason for hiding this comment

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

1 question, looks good otherwise.

Wondering about check in: do not we have to use Version.Current and change it to 7.3.0 in a followup to not break BWC?

@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeInt(docs.size());
for (Map<String, Object> doc : docs) {
out.writeMapWithConsistentOrder(doc);
}
if (out.getVersion().onOrAfter(Version.V_7_3_0)) {
out.writeMap(mappings);

Choose a reason for hiding this comment

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

it seems mappings can be null, so would it make sense to use optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

@hendrikmuhs writeMap and readMap when it is just a Map<String,Object> handles null values internally.

There are also no BWC tests yet...still waiting on PR: #43506

Choose a reason for hiding this comment

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

ok checked it, implicit magic, well...

Copy link

@hendrikmuhs hendrikmuhs left a comment

Choose a reason for hiding this comment

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

LGTM

@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeInt(docs.size());
for (Map<String, Object> doc : docs) {
out.writeMapWithConsistentOrder(doc);
}
if (out.getVersion().onOrAfter(Version.V_7_3_0)) {
out.writeMap(mappings);

Choose a reason for hiding this comment

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

ok checked it, implicit magic, well...

@benwtrent
Copy link
Member Author

run elasticsearch-ci/1
run elasticsearch-ci/packaging-sample

@benwtrent
Copy link
Member Author

@elasticmachine update branch

@benwtrent benwtrent merged commit eea47bb into elastic:master Jul 2, 2019
@benwtrent benwtrent deleted the feature/ml-df-add-mappings-to-_preview branch July 2, 2019 01:23
benwtrent added a commit to benwtrent/elasticsearch that referenced this pull request Jul 2, 2019
…lastic#43742)

* [ML][Data Frame] Add deduced mappings to _preview response payload

* updating preview docs
benwtrent added a commit that referenced this pull request Jul 2, 2019
…oad (#43742) (#43849)

* [ML][Data Frame] Add deduced mappings to _preview response payload (#43742)

* [ML][Data Frame] Add deduced mappings to _preview response payload

* updating preview docs

* fixing code for backport
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