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.

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

Avoid deprecation warning when running the ML datafeed extractor. #31463

Merged
merged 3 commits into from
Jun 22, 2018
Merged

Avoid deprecation warning when running the ML datafeed extractor. #31463

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4645136
Avoid deprecation warning when running the ML datafeed extractor.
jpountz Jun 20, 2018
c3dedda
iter
jpountz Jun 21, 2018
70d910f
Merge branch 'master' into fix/ml_datafeed_extractor
jpountz Jun 21, 2018
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
8 changes: 7 additions & 1 deletion ...ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/extractor/scroll/ExtractedField.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,13 @@ public Object[] value(SearchHit hit) {
if (value.length != 1) {
return value;
}
value[0] = ((BaseDateTime) value[0]).getMillis();
if (value[0] instanceof BaseDateTime) { // script field
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we swap the order of those instanceof checks? The time value will be coming from a doc_value way more often than from a script field, so I'd rather do the most useful check first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

value[0] = ((BaseDateTime) value[0]).getMillis();
} else if (value[0] instanceof String) { // doc_value field with the epoch_millis format
value[0] = Long.parseLong((String) value[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the value going to be a String rather than a long here since we're explicitly setting the format to epoch_millis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is correct.

} else {
throw new IllegalStateException("Unexpected value for a time field: " + value[0].getClass());
}
return value;
}
}
Expand Down
8 changes: 7 additions & 1 deletion ...c/main/java/org/elasticsearch/xpack/ml/datafeed/extractor/scroll/ScrollDataExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.elasticsearch.script.Script;
import org.elasticsearch.search.SearchHit;
import org.elasticsearch.search.fetch.StoredFieldsContext;
import org.elasticsearch.search.fetch.subphase.DocValueFieldsContext;
import org.elasticsearch.search.sort.SortOrder;
import org.elasticsearch.xpack.core.ClientHelper;
import org.elasticsearch.xpack.core.ml.datafeed.extractor.DataExtractor;
Expand Down Expand Up @@ -47,6 +48,7 @@ class ScrollDataExtractor implements DataExtractor {

private static final Logger LOGGER = Loggers.getLogger(ScrollDataExtractor.class);
private static final TimeValue SCROLL_TIMEOUT = new TimeValue(30, TimeUnit.MINUTES);
private static final String EPOCH_MILLIS_FORMAT = "epoch_millis";

private final Client client;
private final ScrollDataExtractorContext context;
Expand Down Expand Up @@ -115,7 +117,11 @@ private SearchRequestBuilder buildSearchRequest(long start) {
context.query, context.extractedFields.timeField(), start, context.end));

for (String docValueField : context.extractedFields.getDocValueFields()) {
searchRequestBuilder.addDocValueField(docValueField);
if (docValueField.equals(context.extractedFields.timeField())) {
searchRequestBuilder.addDocValueField(docValueField, EPOCH_MILLIS_FORMAT);
} else {
searchRequestBuilder.addDocValueField(docValueField, DocValueFieldsContext.USE_DEFAULT_FORMAT);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the default format for date fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On 6.x, the default is to keep returning a date time object for backward compatibility. If you use format=use_field_mapping then date fields will use the format that is configured in the mappings. The default format is strict_date_optional_time||epoch_millis. All formats may be used for parsing but only the first one is used for rendering, so this would give something like 2015-01-01T12:10:30Z.

In 7.x I plan to make use_field_mapping the default via #30831 which is how I found this bug. So ML needs to explicitly request that dates are formatted with epoch_millis in order to get millis since Epoch.

}
}
String[] sourceFields = context.extractedFields.getSourceFields();
if (sourceFields.length == 0) {
Expand Down