Skip to content

Conversation

@panbingkun
Copy link
Contributor

What changes were proposed in this pull request?

This PR aims to replace 'intercept' with 'Check error classes' in DataSourceV2SQLSuite.

Why are the changes needed?

The changes improve the error framework.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

By running the modified test suite:

$ build/sbt "test:testOnly *DataSourceV2SQLSuite"

@github-actions github-actions bot added the SQL label Oct 30, 2022
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@panbingkun
Copy link
Contributor Author

cc @MaxGekk

parameters = Map("cmd" -> sqlCommand))
}

private def assertAnalysisErrorClass(
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove assertAnalysisErrorClass(), and use checkError() directly. You could introduce an method analysisException as we did before for ParseException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 68 to 76
errorClass: String,
parameters: Map[String, String]): Unit = {
checkError(
exception = intercept[AnalysisException] {
sql(sqlStatement)
},
errorClass = errorClass,
parameters = parameters
)
Copy link
Member

Choose a reason for hiding this comment

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

Let's introduce a helper method for this:

      exception = intercept[AnalysisException] {
        sql(sqlStatement)
      },

and invoke checkError directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

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

Waiting for CI.

@MaxGekk
Copy link
Member

MaxGekk commented Oct 31, 2022

@panbingkun Please, fix the code style issue:

[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala: Expected token RPAREN but got Token(EQUALS,=,76130,=)

@panbingkun
Copy link
Contributor Author

@panbingkun Please, fix the code style issue:

[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala: Expected token RPAREN but got Token(EQUALS,=,76130,=)

Done.

@MaxGekk
Copy link
Member

MaxGekk commented Oct 31, 2022

It seems this is related to your changes:

DataSourceV2SQLSuiteV1Filter.MERGE INTO TABLE
org.scalatest.exceptions.TestFailedException: "_LEGACY_ERROR_TEMP_2309" did not equal null

Most likely after 3c967f0, need to merge/rebase the master.

@panbingkun
Copy link
Contributor Author

It seems this is related to your changes:

DataSourceV2SQLSuiteV1Filter.MERGE INTO TABLE
org.scalatest.exceptions.TestFailedException: "_LEGACY_ERROR_TEMP_2309" did not equal null

Most likely after 3c967f0, need to merge/rebase the master.

Done, waiting for CI.

@MaxGekk
Copy link
Member

MaxGekk commented Nov 1, 2022

+1, LGTM. Merging to master. I ran the test locally.
Thank you, @panbingkun.

@MaxGekk MaxGekk closed this in ba019fd Nov 1, 2022
@panbingkun panbingkun deleted the SPARK-40890 branch November 7, 2022 01:51
SandishKumarHN pushed a commit to SandishKumarHN/spark that referenced this pull request Dec 12, 2022
### What changes were proposed in this pull request?
This PR aims to replace 'intercept' with 'Check error classes' in DataSourceV2SQLSuite.

### Why are the changes needed?
The changes improve the error framework.

### Does this PR introduce _any_ user-facing change?
No.

### How was this patch tested?
By running the modified test suite:
```
$ build/sbt "test:testOnly *DataSourceV2SQLSuite"
```

Closes apache#38439 from panbingkun/SPARK-40890.

Authored-by: panbingkun <pbk1982@gmail.com>
Signed-off-by: Max Gekk <max.gekk@gmail.com>
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