Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Oct 11, 2022

What changes were proposed in this pull request?

In the PR, I propose to use checkError() to check the error class, message parameters and the query context. After the PR #37916, all parsing exceptions should have an error class.

Why are the changes needed?

  1. Checking of the error classes plus the query context improves the test coverage.
  2. Eliminating the dependency of error messages. So, tech editors will be able to modify error-classes.json w/o fixing the test suite.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

By running the modified test suite:

$ build/sbt "test:testOnly *ErrorParserSuite"

@github-actions github-actions bot added the SQL label Oct 11, 2022
@MaxGekk MaxGekk changed the title [WIP][SQL][TESTS] Check error classes in ErrorParserSuite [WIP][SPARK-40762][SQL][TESTS] Check error classes in ErrorParserSuite Oct 11, 2022
@MaxGekk MaxGekk changed the title [WIP][SPARK-40762][SQL][TESTS] Check error classes in ErrorParserSuite [SPARK-40762][SQL][TESTS] Check error classes in ErrorParserSuite Oct 11, 2022
@MaxGekk MaxGekk changed the title [SPARK-40762][SQL][TESTS] Check error classes in ErrorParserSuite [SPARK-40762][SQL][TESTS] Check error classes in ErrorParserSuite Oct 11, 2022
@MaxGekk MaxGekk marked this pull request as ready for review October 11, 2022 17:10
@MaxGekk
Copy link
Member Author

MaxGekk commented Oct 11, 2022

@cloud-fan @HyukjinKwon @gengliangwang @srielau @anchovYu @entong We have already migrated all parsing and compilation errors onto error classes but some of test suites don't use checkError, and they check error message using custom helper methods. I would propose to consider this PR as a reference impl. So, after it is merged, I am going to open more JIRAs to migrate rest test suites on checkError(). In this way, we won't depend on error messages in tests.

@MaxGekk
Copy link
Member Author

MaxGekk commented Oct 12, 2022

Merging to master. Thank you, @cloud-fan for review.

@MaxGekk MaxGekk closed this in 0dd7834 Oct 12, 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.

2 participants