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

[SPARK-31725][CORE][SQL][TESTS] Set America/Los_Angeles time zone and Locale.US in tests by default #28548

Closed

Conversation

MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented May 15, 2020

What changes were proposed in this pull request?

Set default time zone and locale in the default constructor of SparkFunSuite:

  • Default time zone to America/Los_Angeles
  • Default locale to Locale.US

Why are the changes needed?

  1. To deduplicate code by moving common time zone and locale settings to one place SparkFunSuite
  2. To have the same default time zone and locale in all tests. This should prevent errors like [SPARK-31526][SQL][TESTS][FOLLOWUP] Make ExpressionInfo independent from local time zone #28538

Does this PR introduce any user-facing change?

No

How was this patch tested?

by running all affected test suites

@MaxGekk MaxGekk changed the title [WIP][SPARK-31725][CORE][SQL][TESTS] Set America/Los_Angeles time zone and Locale.US by default [WIP][SPARK-31725][CORE][SQL][TESTS] Set America/Los_Angeles time zone and Locale.US in tests by default May 15, 2020
@SparkQA
Copy link

SparkQA commented May 15, 2020

Test build #122691 has finished for PR 28548 at commit 591a097.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@MaxGekk
Copy link
Member Author

MaxGekk commented May 15, 2020

jenkins, retest this, please

@MaxGekk MaxGekk changed the title [WIP][SPARK-31725][CORE][SQL][TESTS] Set America/Los_Angeles time zone and Locale.US in tests by default [SPARK-31725][CORE][SQL][TESTS] Set America/Los_Angeles time zone and Locale.US in tests by default May 15, 2020
@SparkQA
Copy link

SparkQA commented May 16, 2020

Test build #122695 has finished for PR 28548 at commit 591a097.

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

@MaxGekk
Copy link
Member Author

MaxGekk commented May 16, 2020

@cloud-fan @HyukjinKwon Please, review this PR.

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Seems OK to me. We still have some tests that exercise other time zones right?

@MaxGekk
Copy link
Member Author

MaxGekk commented May 16, 2020

We still have some tests that exercise other time zones right?

@srowen Right, there is this method DateTimeTestUtils.withDefaultTimeZone which allows to set a time zone temporary:

$  find . -name "*Suite.scala" -print0|xargs -0 grep -l withDefaultTimeZone
./external/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala
./sql/core/src/test/scala/org/apache/spark/sql/DataFrameFunctionsSuite.scala
./sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala
./sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/RebaseDateTimeSuite.scala
./sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala
./sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala
./sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala
./sql/catalyst/src/test/scala/org/apache/spark/sql/util/TimestampFormatterSuite.scala

As far as I know there are no test suites that assume a default time zone that is different from America/Los_Angeles.

@cloud-fan
Copy link
Contributor

thanks, merging to master/3.0!

cloud-fan pushed a commit that referenced this pull request May 17, 2020
… Locale.US in tests by default

### What changes were proposed in this pull request?
Set default time zone and locale in the default constructor of `SparkFunSuite`:
- Default time zone to `America/Los_Angeles`
- Default locale to `Locale.US`

### Why are the changes needed?
1. To deduplicate code by moving common time zone and locale settings to one place SparkFunSuite
2. To have the same default time zone and locale in all tests. This should prevent errors like #28538

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
by running all affected test suites

Closes #28548 from MaxGekk/timezone-settings-SparkFunSuite.

Authored-by: Max Gekk <max.gekk@gmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
(cherry picked from commit 5539ecf)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@cloud-fan cloud-fan closed this in 5539ecf May 17, 2020
@MaxGekk MaxGekk deleted the timezone-settings-SparkFunSuite branch June 5, 2020 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants