Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Aug 1, 2022

What changes were proposed in this pull request?

  1. Re-implemented validateParsingError() using checkError().
  2. Removed checkParsingError() and replaced by checkError().

Why are the changes needed?

  1. To prepare test infra for testing of query contexts.
  2. To check message parameters instead of entire text message. This PR is some kind of follow up of [SPARK-39349] Add a centralized CheckError method for QA of error path #36693 and [SPARK-39905][SQL][TESTS] Remove checkErrorClass() and use checkError() instead #37322.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

By running the modified test suites:

$ build/sbt -Phive -Phive-thriftserver "test:testOnly *TruncateTableParserSuite"
$ build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *ShowPartitionsParserSuite"
$ build/sbt "sql/testOnly *QueryParsingErrorsSuite"

@github-actions github-actions bot added the SQL label Aug 1, 2022
@MaxGekk MaxGekk changed the title [SPARK-39935][SQL][TESTS] Switch validateParsingError() onto checkError() [WIP][SPARK-39935][SQL][TESTS] Switch validateParsingError() onto checkError() Aug 1, 2022
@MaxGekk MaxGekk changed the title [WIP][SPARK-39935][SQL][TESTS] Switch validateParsingError() onto checkError() [SPARK-39935][SQL][TESTS] Switch validateParsingError() onto checkError() Aug 1, 2022
@MaxGekk
Copy link
Member Author

MaxGekk commented Aug 1, 2022

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

Copy link
Contributor

@cloud-fan cloud-fan left a comment

Choose a reason for hiding this comment

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

LGTM, please fix code conflicts

@MaxGekk
Copy link
Member Author

MaxGekk commented Aug 2, 2022

I think the SparkR failure is not related to the changes. I am going to merge this PR to master. Thank you, @cloud-fan @HyukjinKwon for review.

@MaxGekk MaxGekk closed this in 99fc389 Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants