-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add search runtime_mappings to datafeed configuration #65606
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
Conversation
|
Pinging @elastic/ml-core (:ml) |
0330176 to
2f982bd
Compare
droberts195
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
I just had a few minor nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to say object instead of map in this error message, as the user reading the message is thinking of the JSON structure rather than the Java structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map is consistent with the language used here
elasticsearch/server/src/main/java/org/elasticsearch/index/mapper/RuntimeFieldType.java
Line 91 in af2f084
| throw new MapperParsingException("Expected map for runtime field [" + fieldName + "] definition but got a " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still wrong - the word "map" does not appear anywhere in this page: https://www.json.org/json-en.html
It sounds like there is going to be a separate PR to correct this throughout the runtime fields code though, so all instances can be fixed in one go.
...plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedJobValidator.java
Outdated
Show resolved
Hide resolved
...plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedJobValidator.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/elasticsearch/xpack/core/ml/job/persistence/ElasticsearchMappingsTests.java
Outdated
Show resolved
Hide resolved
...-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/ml/integration/DatafeedJobsIT.java
Outdated
Show resolved
Hide resolved
This reverts commit 9233485160f416a15187941f19912c7e58f2a105.
9333c1a to
9f40127
Compare
droberts195
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds the runtime_mappings section to DatafeedConfig to define runtime fields. The runtime fields are then passed through to datafeed searches
For the changes in elastic#65606
This change adds runtime fields in search to the datafeed.
The DatafeedConfig has an optional section to defined search runtime fields
The actual configuration is just a loose
Map<String, Object>as this is whatSearchSourceBuilderuses. There is some validation on PUT that the map contents resemble a runtime_mappings configuration.Datafeed preview and running a datafeed are not exactly the same. The checks in
DatafeedJobValidatorare not run in preview which means the check that the time field is not an RT field will not happen. We should discuss changing preview to exactly match the behaviour or running a datafeed.There are some design decisions that the reviewer should know about:
runtime_mappingsto create the job time field will be rejected.HLRC to follow, this change is big enough.