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

[FEATURE][ML] Add setting for results field name #40927

Conversation

dimitris-athanasiou
Copy link
Contributor

Adds a field in the config dest.results_field which
defaults to ml. Results will be written into an object
that is named after the results field. In addition,
upon starting the analytics there is now validation that
the source index does not already have a field matching
the results field name.

This allows composability of different analytics runs.

@dimitris-athanasiou dimitris-athanasiou added :ml Machine learning :ml/Transform Transform labels Apr 7, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

@dimitris-athanasiou
Copy link
Contributor Author

This depends on elastic/ml-cpp#454 to be merged in

Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

One minor comment about null values.

parser.declareString(ConstructingObjectParser.constructorArg(), INDEX);
parser.declareStringOrNull(ConstructingObjectParser.optionalConstructorArg(), RESULTS_FIELD);
Copy link
Member

Choose a reason for hiding this comment

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

declareStringOrNull I think implies that the passed argument could be explicitly null. This means results_field: null is a valid payload. This does not make sense to me. It seems to be that it being optional is good so we can set the default, but allowing it to be explicitly null and then overwriting it seems strange to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes! Good spot! I got tricked to think it was the method to use for optional fields.

@droberts195
Copy link
Contributor

Jenkins run elasticsearch-ci/2

@droberts195
Copy link
Contributor

Jenkins run elasticsearch-ci/packaging-sample

@dimitris-athanasiou
Copy link
Contributor Author

run elasticsearch-ci/2

@dimitris-athanasiou dimitris-athanasiou force-pushed the add-setting-for-results-field-name branch 2 times, most recently from 7ae73a3 to e731d06 Compare April 11, 2019 10:21
Adds a field in the config `dest.results_field` which
defaults to `ml`. Results will be written into an object
that is named after the results field. In addition,
upon starting the analytics there is now validation that
the source index does not already have a field matching
the results field name.

This allows composability of different analytics runs.
@dimitris-athanasiou dimitris-athanasiou merged commit d230a07 into elastic:feature-ml-data-frame-analytics Apr 11, 2019
@dimitris-athanasiou dimitris-athanasiou deleted the add-setting-for-results-field-name branch April 11, 2019 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ml/Transform Transform :ml Machine learning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants