Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented May 17, 2021

What changes were proposed in this pull request?

Add tests to check the EXCEPTION rebase mode explicitly in the datasources:

  • Parquet: DATE type and TIMESTAMP: INT96, TIMESTAMP_MICROS, TIMESTAMP_MILLIS
  • Avro: DATE type and TIMESTAMP: timestamp-millis and timestamp-micros.

Why are the changes needed?

  1. To improve test coverage
  2. The EXCEPTION rebase mode should be checked independently from the default settings.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By running the affected test suites:

$ build/sbt "test:testOnly *AvroV2Suite"
$ build/sbt "test:testOnly *ParquetRebaseDatetimeV1Suite"

@MaxGekk
Copy link
Member Author

MaxGekk commented May 17, 2021

@cloud-fan Could you review this PR, please.

}
}

test("SPARK-35427: datetime rebasing in the EXCEPTION mode") {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to clean up the existing tests to remove code that are being tested here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me find out the places where we check the default mode (EXCEPTION).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can avoid checking the default EXCEPTION mode at least in the places:

  1. // By default we should fail to write ancient datetime values.
    if (tsOutputType != "INT96") {
    val e = intercept[SparkException](df.write.parquet(path3_0))
    assert(e.getCause.getCause.getCause.isInstanceOf[SparkUpgradeException])
    }
  2. // By default we should fail to write ancient datetime values.
    val e = intercept[SparkException](df.write.format("avro").save(path3_0))
    assert(e.getCause.getCause.getCause.isInstanceOf[SparkUpgradeException])

@cloud-fan Let me know if you think that there are other places.

@SparkQA
Copy link

SparkQA commented May 17, 2021

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43158/

@SparkQA
Copy link

SparkQA commented May 17, 2021

Test build #138638 has finished for PR 32574 at commit 172f83c.

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

@SparkQA
Copy link

SparkQA commented May 20, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43279/

@SparkQA
Copy link

SparkQA commented May 20, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43279/

@SparkQA
Copy link

SparkQA commented May 20, 2021

Test build #138757 has finished for PR 32574 at commit d6e38bd.

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

@MaxGekk
Copy link
Member Author

MaxGekk commented May 21, 2021

@cloud-fan Any objections to the changes?

@MaxGekk
Copy link
Member Author

MaxGekk commented May 21, 2021

@gengliangwang @HyukjinKwon Could you take a look at the PR, please.

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 2b08070 May 21, 2021
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.

3 participants