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

Format support for script doc fields #60465

Merged

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Jul 30, 2020

Adds format and locale support to runtime_script fields,
specifically those with the runtime_type of date. Others
runtime_types will return an error if provided with those parameters.

Relates to #59332

Adds `format` and `locale` support to `runtime_script` fields,
specifically those with the `runtime_type` of `date`. Others
`runtime_type`s will return an error if provided with those parameters.
@nik9000 nik9000 added the :Search/Search Search-related issues that do not fall into other categories label Jul 30, 2020
@nik9000 nik9000 requested a review from javanna July 30, 2020 15:26
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Jul 30, 2020
@javanna javanna mentioned this pull request Jul 30, 2020
30 tasks
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

left a couple of comments, LGTM otherwise

MapperParsingException.class,
() -> createIndex("test", Settings.EMPTY, mapping(runtimeType, b -> b.field("format", "yyyy-MM-dd")))
);
assertThat(e.getMessage(), equalTo("Failed to parse mapping: format not supported by runtime_type [" + runtimeType + "]"));
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think that looking at this error message, there is a slight change that it makes users think that the specific value they provided for the format parameter is not supported, more than providing any format. same for locale. Maybe we can rephrase it?

private void checkBadDate(ThrowingRunnable queryBuilder) {
Exception e = expectThrows(ElasticsearchParseException.class, queryBuilder);
assertThat(e.getMessage(), containsString("failed to parse date field"));
}
Copy link
Member

Choose a reason for hiding this comment

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

would it make sense to have some duel tests between concrete dates and runtime dates to make sure that formatting and locale are applied the same way?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah!

@nik9000 nik9000 merged commit 0e91859 into elastic:feature/runtime_fields Aug 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants