Skip to content

[CI] Fix year of week formatting when using joda time #66914

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

Merged
merged 3 commits into from
Jan 4, 2021

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Jan 4, 2021

Based on the joda-time documentation, the symbol for "year of the week" is x as opposed to Y (used by java's DateTimeFormatter). This PR fixes the test code to use the correct date formatting symbol.

Resolves: #66907

@ywangd ywangd added >test Issues or PRs that are addressing/adding tests :Core/Infra/Core Core issues without another label v8.0.0 v7.12.0 labels Jan 4, 2021
@ywangd ywangd requested a review from rjernst January 4, 2021 06:53
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jan 4, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@romseygeek
Copy link
Contributor

I'm not sure this is the correct fix. Why is {now/M{YYYY.MM}} getting resolved to 2020.01 when we're in 2021?

@romseygeek
Copy link
Contributor

OK, having read https://www.alexcorrigan.co.uk/blog/20191231-0644-javas-week-based-year-datetimeformatter/ I understand what the issue is - {now/M{YYYY.MM}} is resolving the week-based year as of the first of January, which falls in a week in which the Thursday is in 2020. I think this might be better fixed by changing the expression to be {now/M{yyyy.MM}} which looks like something a user might actually want to do?

Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

This test is based on joda format (was possibly missed when migrating) and Y meant year of era in joda.
It should be changed to uuuu when using java.time

@pgomulka
Copy link
Contributor

pgomulka commented Jan 4, 2021

I'm not sure this is the correct fix. Why is {now/M{YYYY.MM}} getting resolved to 2020.01 when we're in 2021?

in week based calendar 01-03.01.2021 are still in 2020. The first week of 2021 starts on Monday 4th. This is because it requires first week to contain minimum 4 days in new year. Also Monday is the first day fo the week per ISO rule.
http://www.whatweekisit.org/

This is all because YYYY (week based year) was used instead of yyyy (year of era) or uuuu (year)

@ywangd
Copy link
Member Author

ywangd commented Jan 4, 2021

This test is based on joda format (was possibly missed when migrating) and Y meant year of era in joda.
It should be changed to uuuu when using java.time

If the intention is to use "year-of-era", I can change the date-math-expression to use yyyy (year-of-era for java DateTimeFormmatter) instead of changing the assertion code, i.e. something like the follows:

@@ -115,7 +115,7 @@ public class DateMathExpressionResolverTests extends ESTestCase {

     public void testExpression_MixedArray() throws Exception {
         List<String> result = expressionResolver.resolve(context, Arrays.asList(
-                "name1", "<.marvel-{now/d}>", "name2", "<.logstash-{now/M{YYYY.MM}}>"
+                "name1", "<.marvel-{now/d}>", "name2", "<.logstash-{now/M{yyyy.MM}}>"
         ));
         assertThat(result.size(), equalTo(4));
         assertThat(result.get(0), equalTo("name1"));

Alternatively, we can change both date-math-expression and the assertion to use simply "year", i.e. uuuu for java DateTimeFormatter and yyyy for the assertion. Which one would you suggest that is closer to the actual intention?

@ywangd ywangd requested a review from pgomulka January 4, 2021 10:47
@pgomulka
Copy link
Contributor

pgomulka commented Jan 4, 2021

@ywangd edit - you are right. it should be uuuu in the date math, and yyyy in assertion as the assertion is still using joda. We should migrate this test in another PR

@ywangd
Copy link
Member Author

ywangd commented Jan 4, 2021

@ywangd edit - you are right. it should be uuuu in the date math, and yyyy in assertion as the assertion is still using joda. We should migrate this test in another PR

Thanks @pgomulka The PR is updated as discussed.

Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ywangd ywangd merged commit 5aa82eb into elastic:master Jan 4, 2021
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Jan 4, 2021
…#66914)

This PR fixes the test failure by ensuring consistent datetime formatting
between java DateTimeFormatter and joda time.
ywangd added a commit that referenced this pull request Jan 4, 2021
…#66925)

This PR fixes the test failure by ensuring consistent datetime formatting
between java DateTimeFormatter and joda time.
benwtrent pushed a commit to benwtrent/elasticsearch that referenced this pull request Jan 4, 2021
…#66914)

This PR fixes the test failure by ensuring consistent datetime formatting
between java DateTimeFormatter and joda time.
benwtrent added a commit that referenced this pull request Jan 4, 2021
…#66932)

This PR fixes the test failure by ensuring consistent datetime formatting
between java DateTimeFormatter and joda time.

Co-authored-by: Yang Wang <[email protected]>
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Jan 5, 2021
…#66914)

This PR fixes the test failure by ensuring consistent datetime formatting
between java DateTimeFormatter and joda time.
ywangd added a commit that referenced this pull request Jan 5, 2021
…#66974)

This PR fixes the test failure by ensuring consistent datetime formatting
between java DateTimeFormatter and joda time.
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team >test Issues or PRs that are addressing/adding tests v7.12.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reproducible Failure in org.elasticsearch.cluster.metadata.DateMathExpressionResolverTests.testExpression_MixedArray
5 participants