Skip to content

[SPARK-31526][SQL][TESTS][FOLLOWUP] Make ExpressionInfo independent from local time zone #28538

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[SPARK-31526][SQL][TESTS][FOLLOWUP] Make ExpressionInfo independent from local time zone #28538

wants to merge 1 commit into from

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented May 15, 2020

What changes were proposed in this pull request?

Extend QueryTest instead of SparkFunSuite in ExpressionInfoSuite as in SQLQuerySuite.

Why are the changes needed?

After the changes #28308, the moved tests from SQLQuerySuite became dependent from local time zone settings. And tests from ExpressionInfoSuite fail if they run not in the America/Los_Angeles time zone:

[info] - check outputs of expression examples *** FAILED *** (7 seconds, 720 milliseconds)
[info]   Function 'from_unixtime', Expression class 'org.apache.spark.sql.catalyst.expressions.FromUnixTime' "19[70-01-01 03]:00:00" did not equal "19[69-12-31 16]:00:00" (ExpressionInfoSuite.scala:152)

Does this PR introduce any user-facing change?

No

How was this patch tested?

By running the modified test suite locally in the Europe/Moscow time zone:

/build/sbt "test:testOnly *ExpressionInfoSuite"

@MaxGekk
Copy link
Member Author

MaxGekk commented May 15, 2020

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

HyukjinKwon
HyukjinKwon approved these changes May 15, 2020
View reviewed changes
Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

LGTM

@SparkQA
Copy link

SparkQA commented May 15, 2020

Test build #122661 has finished for PR 28538 at commit db11dcc.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class ExpressionInfoSuite extends QueryTest with SharedSparkSession

@MaxGekk
Copy link
Member Author

MaxGekk commented May 15, 2020

The failure in org.apache.spark.sql.execution.ui.SQLAppStatusListenerSuite.driver side SQL metrics shouldn't be caused by the changes.

@MaxGekk
Copy link
Member Author

MaxGekk commented May 15, 2020

jenkins, retest this, please

@SparkQA
Copy link

SparkQA commented May 15, 2020

Test build #122671 has finished for PR 28538 at commit db11dcc.

  • This patch fails from timeout after a configured wait of 400m.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class ExpressionInfoSuite extends QueryTest with SharedSparkSession

@MaxGekk MaxGekk mentioned this pull request May 15, 2020
Closed
@MaxGekk
Copy link
Member Author

MaxGekk commented May 15, 2020

jenkins, retest this, please

@SparkQA
Copy link

SparkQA commented May 16, 2020

Test build #122692 has finished for PR 28538 at commit db11dcc.

  • This patch fails from timeout after a configured wait of 400m.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class ExpressionInfoSuite extends QueryTest with SharedSparkSession

@MaxGekk
Copy link
Member Author

MaxGekk commented May 16, 2020

jenkins, retest this, please

@maropu
Copy link
Member

maropu commented May 16, 2020

hm, pretty flaky...

@SparkQA
Copy link

SparkQA commented May 16, 2020

Test build #122702 has finished for PR 28538 at commit db11dcc.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class ExpressionInfoSuite extends QueryTest with SharedSparkSession

@MaxGekk
Copy link
Member Author

MaxGekk commented May 16, 2020

jenkins, retest this, please

@SparkQA
Copy link

SparkQA commented May 16, 2020

Test build #122720 has finished for PR 28538 at commit db11dcc.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class ExpressionInfoSuite extends QueryTest with SharedSparkSession

cloud-fan pushed a commit that referenced this pull request May 17, 2020
@MaxGekk @cloud-fan
[SPARK-31725][CORE][SQL][TESTS] Set America/Los_Angeles time zone and…
4f5df2c 
… 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 <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
(cherry picked from commit 5539ecf)
Signed-off-by: Wenchen Fan <[email protected]>
cloud-fan pushed a commit that referenced this pull request May 17, 2020
@MaxGekk @cloud-fan
[SPARK-31725][CORE][SQL][TESTS] Set America/Los_Angeles time zone and…
5539ecf 
… 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 <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented May 17, 2020

Since SPARK-31725 improves SparkFunSuite already, I'll close this PR.
Please feel free to reopen this if we still need this, @MaxGekk . Thanks.

@maropu maropu mentioned this pull request Jun 2, 2020
Closed
@MaxGekk MaxGekk deleted the fix-ExpressionInfoSuite branch June 5, 2020 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maropu maropu maropu approved these changes

@gengliangwang gengliangwang gengliangwang approved these changes

@cloud-fan cloud-fan cloud-fan approved these changes

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@MaxGekk @SparkQA @maropu @dongjoon-hyun @gengliangwang @cloud-fan @HyukjinKwon