[SPARK-25670][TEST] Reduce number of tested timezones in JsonExpressionsSuite #22657

MaxGekk · 2018-10-06T14:00:36Z

What changes were proposed in this pull request?

After the changes, total execution time of JsonExpressionsSuite.scala dropped from 12.5 seconds to 3 seconds.

srowen · 2018-10-06T17:41:13Z

-1 We definitely don't want to randomly test subsets of functionality just to make things faster. 9 seconds isn't worth it.

SparkQA · 2018-10-06T18:04:34Z

Test build #97051 has finished for PR 22657 at commit e7d17d2.

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

MaxGekk · 2018-10-06T18:36:05Z

@srowen What is the difference between this test suite and this PR #22631 . Also I took a sub-set of timezones in the PR: #22379 (comment) . I think we should apply consistent approach across all test suites. @gatorsmile @cloud-fan What do you think?

9 seconds isn't worth it.

It isn't worth if you run the test not so often. I run the test suite manually pretty often, and for me it is worth.

srowen · 2018-10-06T18:43:08Z

I didn't see that one, and I object to it. I'll follow up there.

MaxGekk · 2018-10-08T13:06:25Z

I didn't see that one, and I object to it.

I believe we don't need to test all timezones in the case of JSON datasource. Actually we just check how FastDateFormat of commons-lang3 is able to parse/generate JSON by using different timezones. From my point of view, It should be enough to have one test case which checks that we properly pass a timezone to FastDateFormat. Don't think we have to test apache commons-lang3 fully. @HyukjinKwon What do you think of it?

HyukjinKwon · 2018-10-08T13:30:15Z

I agree. We don't test commons-lang3 library here - one test should be good enough since what we need to check is if it delegates and passes the timezone properly or not.

cloud-fan · 2018-10-08T17:40:18Z

Then we don't need any randomness here, just pick one timezone(like PST?) and test it.

cloud-fan · 2018-10-10T13:56:49Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeTestUtils.scala


  val ALL_TIMEZONES: Seq[TimeZone] = TimeZone.getAvailableIDs.toSeq.map(TimeZone.getTimeZone)

+  val TIMEZONES = Seq(


shall we move it to SparkFunSuite and name it like outstandingTimezones?

It seems it should be useful across a few test suites. I will do that.

MaxGekk · 2018-10-10T13:57:17Z

Then we don't need any randomness here, just pick one timezone(like PST?) and test it.

I took 8 out of 627. Going to do the same in the PR: #22379

srowen · 2018-10-10T16:29:24Z

core/src/test/scala/org/apache/spark/SparkFunSuite.scala

    }
  }

+  lazy val outstandingTimezones = Seq(


Is this lazy so that it's not evaluated by each test suite? I get it although TimeZone caches these, it seems. It won't matter really either way. I'm neutral, it doesn't matter

SparkQA · 2018-10-10T17:41:13Z

Test build #97201 has finished for PR 22657 at commit 5f8bf79.

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

SparkQA · 2018-10-10T18:39:54Z

Test build #97202 has finished for PR 22657 at commit 778629d.

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

srowen

This goes from 14 seconds to almost no time. That's a small nice win. Where else can we apply this subset?

kiszk · 2018-10-11T07:37:31Z

Do we need to apply the same reduction to CastSuite?
I believe that DateTimeUtilsSuite should check all of the time zones, but may skip some values between-20000 to 20000.

MaxGekk · 2018-10-11T07:47:14Z

Where else can we apply this subset?

I am going to make similar changes in tests for from_csv where we don't need to test all timezones too: https://github.com/apache/spark/pull/22379/files#diff-95f5ea298edecd7edf5578466969d7eeR93

kiszk · 2018-10-11T08:06:00Z

How about defining this subset in object DateTimeTestUtils like ALL_TIMEZONES?

MaxGekk · 2018-10-11T08:22:21Z

How about defining this subset in object DateTimeTestUtils like ALL_TIMEZONES?

This is what I already did in the PR: #22657 (comment)

cloud-fan · 2018-10-11T11:32:38Z

If it's only used in test let's define it in the test scope.

MaxGekk · 2018-10-11T11:48:49Z

let's define it in the test scope.

What's the test scope here? Both DateTimeTestUtils and SparkFunSuite are used in test suites only.

cloud-fan · 2018-10-11T12:13:27Z

Ah sorry I misread it as DateTimeUtils before...

Then yes, it's a better place. Sorry for the back and forth!

MaxGekk · 2018-10-11T12:15:11Z

Then yes, it's a better place. Sorry for the back and forth!

@cloud-fan Never mind. I will return it back. Thank you for reviewing it.

SparkQA · 2018-10-11T16:19:56Z

Test build #97259 has finished for PR 22657 at commit 30be9f3.

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

kiszk · 2018-10-11T16:21:45Z

Sorry for bothering you again. Do we need to apply the same reduction to CastSuite?

MaxGekk · 2018-10-11T16:39:57Z

@kiszk It has been already parallelized by @srowen:

spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

Line 115 in eaafcd8

ALL_TIMEZONES.par.foreach { tz =>

. What is the reason to reduce the number of zones there?

srowen · 2018-10-11T16:46:49Z

Yeah, I could see the argument both ways for keeping all the tests in CastSuite or just checking a subset. We already got the test down considerably, though it's still like 24 seconds. Is there any reasonable reason to think the subset of timezones doesn't cover a case we can imagine that could fail here?

srowen · 2018-10-12T17:39:04Z

Merged to master

MaxGekk · 2018-10-12T17:48:16Z

@srowen @HyukjinKwon @cloud-fan Thank you for your review of the PR.

…onsSuite ## What changes were proposed in this pull request? After the changes, total execution time of `JsonExpressionsSuite.scala` dropped from 12.5 seconds to 3 seconds. Closes apache#22657 from MaxGekk/json-timezone-test. Authored-by: Maxim Gekk <maxim.gekk@databricks.com> Signed-off-by: Sean Owen <sean.owen@databricks.com>

Reduce number of timezones in the test

e7d17d2

MaxGekk changed the title ~~[SPARK-25670][SQL] Reduce number of tested timezones in JsonExpressionsSuite~~ [SPARK-25670][TEST] Reduce number of tested timezones in JsonExpressionsSuite Oct 6, 2018

Taking a few out of all timezones for testing

5f8bf79

cloud-fan reviewed Oct 10, 2018

View reviewed changes

Moving timezones to SparkFunSuite

778629d

srowen approved these changes Oct 10, 2018

View reviewed changes

MaxGekk added 2 commits October 11, 2018 14:17

Merge remote-tracking branch 'origin/master' into json-timezone-test

4322897

Moving back to DataTimeTestUtils

30be9f3

HyukjinKwon approved these changes Oct 11, 2018

View reviewed changes

asfgit closed this in d47a25f Oct 12, 2018

MaxGekk deleted the json-timezone-test branch August 17, 2019 13:35

MaxGekk mentioned this pull request Mar 27, 2020

[SPARK-31277][SQL][TESTS] Migrate DateTimeTestUtils from TimeZone to ZoneId #28033

Closed


		val ALL_TIMEZONES: Seq[TimeZone] = TimeZone.getAvailableIDs.toSeq.map(TimeZone.getTimeZone)

		val TIMEZONES = Seq(

[SPARK-25670][TEST] Reduce number of tested timezones in JsonExpressionsSuite #22657

[SPARK-25670][TEST] Reduce number of tested timezones in JsonExpressionsSuite #22657

Uh oh!

Conversation

MaxGekk commented Oct 6, 2018

What changes were proposed in this pull request?

Uh oh!

srowen commented Oct 6, 2018

Uh oh!

SparkQA commented Oct 6, 2018

Uh oh!

MaxGekk commented Oct 6, 2018

Uh oh!

srowen commented Oct 6, 2018

Uh oh!

MaxGekk commented Oct 8, 2018

Uh oh!

HyukjinKwon commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Oct 8, 2018

Uh oh!

cloud-fan Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

MaxGekk Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Oct 10, 2018

Uh oh!

srowen Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 10, 2018

Uh oh!

SparkQA commented Oct 10, 2018

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

kiszk commented Oct 11, 2018

Uh oh!

MaxGekk commented Oct 11, 2018

Uh oh!

kiszk commented Oct 11, 2018

Uh oh!

MaxGekk commented Oct 11, 2018

Uh oh!

cloud-fan commented Oct 11, 2018

Uh oh!

MaxGekk commented Oct 11, 2018

Uh oh!

cloud-fan commented Oct 11, 2018

Uh oh!

MaxGekk commented Oct 11, 2018

Uh oh!

SparkQA commented Oct 11, 2018

Uh oh!

kiszk commented Oct 11, 2018

Uh oh!

MaxGekk commented Oct 11, 2018

Uh oh!

srowen commented Oct 11, 2018

Uh oh!

srowen commented Oct 12, 2018

Uh oh!

MaxGekk commented Oct 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

HyukjinKwon commented Oct 8, 2018 •

edited

Loading