[SPARK-31879][SQL][test-java11] Preset monday start(iso) for the first day of week by reflection #28727

yaooqinn · 2020-06-04T10:02:33Z

What changes were proposed in this pull request?

This PR set a cache entry for the key[fisrtdayofweek + minimalDaysInFirstWeek] (Sunday +1) with a default value java.time.temporal.WeekFields.ISO to provide Monday as the start of week system-widely.

The default locale and custom locale will all obey this rule. so the meaning of u for localized can be removed in doc as it does not change via different locales, which makes it closer to 2.4.

Both #28692 and #28719 have limitations as mentioned in their discussions.

Why are the changes needed?

These week-based fields need Locale to express their semantics, the first day of the week varies from country to country.

From the Java doc of WeekFields

    /**
     * Gets the first day-of-week.
     * <p>
     * The first day-of-week varies by culture.
     * For example, the US uses Sunday, while France and the ISO-8601 standard use Monday.
     * This method returns the first day using the standard {@code DayOfWeek} enum.
     *
     * @return the first day-of-week, not null
     */
    public DayOfWeek getFirstDayOfWeek() {
        return firstDayOfWeek;
    }

But for the SimpleDateFormat, the day-of-week is not localized

u	Day number of week (1 = Monday, ..., 7 = Sunday)	Number	1

Currently, the default locale we use is the US, so the result moved a day backward.

For other countries, please refer to First Day of the Week in Different Countries

With this change, it restores the first day of week calculating for functions when using the default locale.

Does this PR introduce any user-facing change?

Yes, the values of week-based items will be fixed, e.g.

For the date 2019-12-29(Sunday), in the Sunday Start system(e.g. en-US), it belongs to 2020 of week-based-year, in the Monday Start system(en-GB), it goes to 2019. the week-of-week-based-year(w) will be affected too

before

spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2019-12-29', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY', 'locale', 'en-US'));
2020
spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2019-12-29', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY', 'locale', 'en-GB'));
2019

spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2019-12-29', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY-ww-uu', 'locale', 'en-US'));
2020-01-01
spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2019-12-29', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY-ww-uu', 'locale', 'en-GB'));
2019-52-07

spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2020-01-05', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY-ww-uu', 'locale', 'en-US'));
2020-02-01
spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2020-01-05', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY-ww-uu', 'locale', 'en-GB'));
2020-01-07

After

spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2019-12-29', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY', 'locale', 'en-US'));
2019
spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2019-12-29', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY', 'locale', 'en-GB'));
2019

spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2019-12-29', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY-ww-uu', 'locale', 'en-US'));
2019-52-07
spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2019-12-29', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY-ww-uu', 'locale', 'en-GB'));
2019-52-07

spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2020-01-05', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY-ww-uu', 'locale', 'en-US'));
2020-01-07
spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2020-01-05', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY-ww-uu', 'locale', 'en-GB'));
2020-01-07

How was this patch tested?

add unit tests

… by reflection

yaooqinn · 2020-06-04T10:16:59Z

retest this please

yaooqinn · 2020-06-04T10:26:42Z

cc @cloud-fan @xuanyuanking @MaxGekk @srowen

cloud-fan · 2020-06-04T12:06:41Z

docs/sql-ref-datetime-pattern.md

 The count of pattern letters determines the format.

- Text: The text style is determined based on the number of pattern letters used. Less than 4 pattern letters will use the short form. Exactly 4 pattern letters will use the full form. Exactly 5 pattern letters will use the narrow form. 5 or more letters will fail.
+- Text: The text style is determined based on the number of pattern letters used. Less than 4 pattern letters will use the short form. Exactly 4 pattern letters will use the full form. Exactly 5 pattern letters will use the narrow form. 5 or more letters will fail. More details for the text style:


How about

... will use the short form (typically an abbreviation, for example, day-of-week Monday might output "Mon"). ...

BTW we should remove Exactly 5 pattern letters will use the narrow form

srowen · 2020-06-04T12:42:47Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala

+  val defaultLocale = Locale.US
+
+  def presetSundayStartToMondayStart(): Unit = {
+    val CACHE: Field = classOf[WeekFields].getDeclaredField("CACHE")


Yikes, isn't this going to change the Locale's settings for the whole JVM?

Only for those trying to use the original value of SUNDAY FISRT and 1 mininalDay. This seems not to be a good way though.

yea, we shouldn't change a global cache, which is shared in the entire JVM, including user code.

SparkQA · 2020-06-04T14:23:15Z

Test build #123528 has finished for PR 28727 at commit 0c3207c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-06-04T14:40:44Z

Test build #123529 has finished for PR 28727 at commit 0c3207c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-06-04T16:52:23Z

Test build #123533 has finished for PR 28727 at commit 590b40a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

…ormatting too # What changes were proposed in this pull request? After all these attempts #28692 and #28719 an #28727. they all have limitations as mentioned in their discussions. Maybe the only way is to forbid them all ### Why are the changes needed? These week-based fields need Locale to express their semantics, the first day of the week varies from country to country. From the Java doc of WeekFields ```java /** * Gets the first day-of-week. * <p> * The first day-of-week varies by culture. * For example, the US uses Sunday, while France and the ISO-8601 standard use Monday. * This method returns the first day using the standard {code DayOfWeek} enum. * * return the first day-of-week, not null */ public DayOfWeek getFirstDayOfWeek() { return firstDayOfWeek; } ``` But for the SimpleDateFormat, the day-of-week is not localized ``` u Day number of week (1 = Monday, ..., 7 = Sunday) Number 1 ``` Currently, the default locale we use is the US, so the result moved a day or a year or a week backward. e.g. For the date `2019-12-29(Sunday)`, in the Sunday Start system(e.g. en-US), it belongs to 2020 of week-based-year, in the Monday Start system(en-GB), it goes to 2019. the week-of-week-based-year(w) will be affected too ```sql spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2019-12-29', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY', 'locale', 'en-US')); 2020 spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2019-12-29', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY', 'locale', 'en-GB')); 2019 spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2019-12-29', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY-ww-uu', 'locale', 'en-US')); 2020-01-01 spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2019-12-29', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY-ww-uu', 'locale', 'en-GB')); 2019-52-07 spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2020-01-05', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY-ww-uu', 'locale', 'en-US')); 2020-02-01 spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2020-01-05', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY-ww-uu', 'locale', 'en-GB')); 2020-01-07 ``` For other countries, please refer to [First Day of the Week in Different Countries](http://chartsbin.com/view/41671) ### Does this PR introduce _any_ user-facing change? With this change, user can not use 'YwuW', but 'e' for 'u' instead. This can at least turn this not to be a silent data change. ### How was this patch tested? add unit tests Closes #28728 from yaooqinn/SPARK-31879-NEW2. Authored-by: Kent Yao <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…ormatting too After all these attempts apache#28692 and apache#28719 an apache#28727. they all have limitations as mentioned in their discussions. Maybe the only way is to forbid them all These week-based fields need Locale to express their semantics, the first day of the week varies from country to country. From the Java doc of WeekFields ```java /** * Gets the first day-of-week. * <p> * The first day-of-week varies by culture. * For example, the US uses Sunday, while France and the ISO-8601 standard use Monday. * This method returns the first day using the standard {code DayOfWeek} enum. * * return the first day-of-week, not null */ public DayOfWeek getFirstDayOfWeek() { return firstDayOfWeek; } ``` But for the SimpleDateFormat, the day-of-week is not localized ``` u Day number of week (1 = Monday, ..., 7 = Sunday) Number 1 ``` Currently, the default locale we use is the US, so the result moved a day or a year or a week backward. e.g. For the date `2019-12-29(Sunday)`, in the Sunday Start system(e.g. en-US), it belongs to 2020 of week-based-year, in the Monday Start system(en-GB), it goes to 2019. the week-of-week-based-year(w) will be affected too ```sql spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2019-12-29', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY', 'locale', 'en-US')); 2020 spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2019-12-29', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY', 'locale', 'en-GB')); 2019 spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2019-12-29', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY-ww-uu', 'locale', 'en-US')); 2020-01-01 spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2019-12-29', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY-ww-uu', 'locale', 'en-GB')); 2019-52-07 spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2020-01-05', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY-ww-uu', 'locale', 'en-US')); 2020-02-01 spark-sql> SELECT to_csv(named_struct('time', to_timestamp('2020-01-05', 'yyyy-MM-dd')), map('timestampFormat', 'YYYY-ww-uu', 'locale', 'en-GB')); 2020-01-07 ``` For other countries, please refer to [First Day of the Week in Different Countries](http://chartsbin.com/view/41671) With this change, user can not use 'YwuW', but 'e' for 'u' instead. This can at least turn this not to be a silent data change. add unit tests Closes apache#28728 from yaooqinn/SPARK-31879-NEW2. Authored-by: Kent Yao <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 9d5b5d0) Signed-off-by: Wenchen Fan <[email protected]>

[SPARK-31879][SQL] Preset monday start(iso) for the first day of week…

0c3207c

… by reflection

probot-autolabeler bot added DOCS SQL labels Jun 4, 2020

yaooqinn changed the title ~~[SPARK-31879][SQL] Preset monday start(iso) for the first day of week by reflection~~ [SPARK-31879][SQL][test-java11] Preset monday start(iso) for the first day of week by reflection Jun 4, 2020

cloud-fan mentioned this pull request Jun 4, 2020

[SPARK-31899][SQL] Forbid datetime pattern letter u #28719

Closed

cloud-fan reviewed Jun 4, 2020

View reviewed changes

update

590b40a

srowen reviewed Jun 4, 2020

View reviewed changes

yaooqinn mentioned this pull request Jun 4, 2020

[SPARK-31879][SQL][test-java11] Make week-based pattern invalid for formatting too #28728

Closed

yaooqinn closed this Jun 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SPARK-31879][SQL][test-java11] Preset monday start(iso) for the first day of week by reflection #28727

[SPARK-31879][SQL][test-java11] Preset monday start(iso) for the first day of week by reflection #28727

Uh oh!

yaooqinn commented Jun 4, 2020 •

edited

Loading

Uh oh!

yaooqinn commented Jun 4, 2020

Uh oh!

yaooqinn commented Jun 4, 2020

Uh oh!

cloud-fan Jun 4, 2020

Uh oh!

srowen Jun 4, 2020

Uh oh!

yaooqinn Jun 4, 2020

Uh oh!

cloud-fan Jun 4, 2020

Uh oh!

SparkQA commented Jun 4, 2020

Uh oh!

SparkQA commented Jun 4, 2020

Uh oh!

SparkQA commented Jun 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[SPARK-31879][SQL][test-java11] Preset monday start(iso) for the first day of week by reflection #28727

[SPARK-31879][SQL][test-java11] Preset monday start(iso) for the first day of week by reflection #28727

Uh oh!

Conversation

yaooqinn commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

before

After

How was this patch tested?

Uh oh!

yaooqinn commented Jun 4, 2020

Uh oh!

yaooqinn commented Jun 4, 2020

Uh oh!

cloud-fan Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

srowen Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

yaooqinn Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 4, 2020

Uh oh!

SparkQA commented Jun 4, 2020

Uh oh!

SparkQA commented Jun 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yaooqinn commented Jun 4, 2020 •

edited

Loading