Skip to content

Conversation

@felixbarny
Copy link
Member

Date detection is not supposed to kick in with numeric values:

} else if (parseableAsLong == false && parseableAsDouble == false && context.root().dateDetection()) {
// We refuse to match pure numbers, which are too likely to be
// false positives with date formats that include eg.
// `epoch_millis` or `YYYY`
for (DateFormatter dateTimeFormatter : context.root().dynamicDateTimeFormatters()) {
try {
dateTimeFormatter.parseMillis(text);
} catch (DateTimeException | ElasticsearchParseException | IllegalArgumentException e) {
// failure to parse this, continue
continue;
}
return createDynamicDateField(
context,
name,
dateTimeFormatter,
() -> strategy.newDynamicDateField(context, name, dateTimeFormatter)
);
}

The way this is ensured is that string values that can be parsed as a number are not subject to date detection.

However, there's a case where a string can't be parsed as a number but is still considered a date that can be parsed by the default epoch_millis formatter.

That's because the epoch_millis format supports parsing numbers with a trailing dot, such as 12345., which is equivalent to 12345.0 or 12345. This was made to retain compatibility with Joda time.

The issue is that the MILLISECONDS_FORMATTER2, built to support trailing dots, is implemented as extension of MILLISECONDS_FORMATTER1, which already supports an optional fractional. The result is that MILLISECONDS_FORMATTER2 supports a fractional and a trailing dot.

The bug got introduced in this PR: #36914.

A potential alternative to this approach would be to remove epoch_millis from the default formatters used in date_detection:

public static final Explicit<DateFormatter[]> DYNAMIC_DATE_TIME_FORMATTERS = new Explicit<>(
new DateFormatter[] {
DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER,
DateFormatter.forPattern("yyyy/MM/dd HH:mm:ss||yyyy/MM/dd||epoch_millis") },
false
);

However, the consequence of this is that it will use a non-default formatter for dynamically detected date fields (strict_date_optional_time instead of strict_date_optional_time||epoch_millis). That means the get mapping API will return "date": { "type": "date", "format": "strict_date_optional_time" } for dynamically mapped dates instead of just "date": { "type": "date" }.

That's why I've opted for a solution that has a lower chance of unintended side effects.

Fixes #116946

@felixbarny felixbarny added >bug :Search Foundations/Mapping Index mappings, including merging and defining field types labels Nov 18, 2024
@elasticsearchmachine elasticsearchmachine added v9.0.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Nov 18, 2024
@elasticsearchmachine
Copy link
Collaborator

Hi @felixbarny, I've created a changelog YAML for you.

@felixbarny felixbarny marked this pull request as ready for review November 18, 2024 15:05
@felixbarny felixbarny requested a review from a team as a code owner November 18, 2024 15:05
@elasticsearchmachine elasticsearchmachine added the Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch label Nov 18, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-foundations (Team:Search Foundations)

@felixbarny felixbarny added v8.16.1 v8.17.0 auto-backport Automatically create backport pull requests when merged and removed Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch labels Nov 18, 2024
@elasticsearchmachine elasticsearchmachine added Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v8.16.2 and removed v8.16.1 labels Nov 18, 2024
Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

Change looks good to me, but you might want to have one approval from the search team to ensure this has no unintended side effects.

@ldematte ldematte requested a review from a team December 2, 2024 09:28
@felixbarny felixbarny added v8.17.1 and removed v8.16.2 labels Dec 4, 2024
@felixbarny felixbarny merged commit 9f581d5 into elastic:main Dec 4, 2024
16 checks passed
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.17
8.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged >bug external-contributor Pull request authored by a developer outside the Elasticsearch team :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v8.17.1 v8.18.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Strings formatted as 128.0. (ending in a decimal) are detected as date

4 participants