Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Jan 24, 2022

What changes were proposed in this pull request?

In the PR, I propose to re-use one error class UNSUPPORTED_FEATURE in the following Spark's exceptions:

  • QueryCompilationErrors.unsupportedIfNotExistsError - when IF NOT EXISTS is not supported by INSERT INTO.
  • QueryExecutionErrors.aesModeUnsupportedError - when an user specify unsupported AES mode and padding.
  • QueryExecutionErrors.literalTypeUnsupportedError - impossible to create a literal from the input value (some Java class, for instance).
  • QueryExecutionErrors.transactionUnsupportedByJdbcServerError - the target JDBC server does not support transaction.

And replace the following exceptions by IllegalStateException since they are internal, and should be not visible to users:

  • QueryExecutionErrors.simpleStringWithNodeIdUnsupportedError - a sub-class of Expression or Block doesn't implements the method simpleStringWithNodeId().
  • QueryExecutionErrors.dataTypeUnsupportedForWriterFuncError - generating of a writer function for a struct field, array element, map key or map value doesn't support Catalyst's type.

Also, added new base test suite QueryCompilationErrorsDSv2Suite for testing DSv2 specific compilation error classes.

Why are the changes needed?

Reducing the number of error classes should prevent from explode of error-classes.json. Also, using one error class for similar errors should improve user experience with Spark SQL.

Does this PR introduce any user-facing change?

Yes.

How was this patch tested?

By running the affected test suites:

$ build/sbt "test:testOnly *SparkThrowableSuite"
$ build/sbt "test:testOnly *QueryExecutionErrorsSuite"
$ build/sbt "test:testOnly *DataSourceV2SQLSuite"
$ build/sbt "test:testOnly *DataSourceV2DataFrameSessionCatalogSuite"
$ build/sbt "test:testOnly *DataFramePivotSuite"

and the new test suite:

$ build/sbt "test:testOnly *QueryCompilationErrorsDSv2Suite"

@MaxGekk MaxGekk changed the title [WIP][SPARK-38001][SQL] Replace the unsupported error classes by UNSUPPORTED_FEATURE [SPARK-38001][SQL] Replace the unsupported error classes by UNSUPPORTED_FEATURE Jan 25, 2022
@MaxGekk MaxGekk marked this pull request as ready for review January 25, 2022 16:01
@MaxGekk MaxGekk requested a review from cloud-fan January 25, 2022 16:01
@MaxGekk MaxGekk changed the title [SPARK-38001][SQL] Replace the unsupported error classes by UNSUPPORTED_FEATURE [SPARK-38001][SQL] Replace the error classes related to unsupported features by UNSUPPORTED_FEATURE Jan 25, 2022

case _ =>
throw QueryExecutionErrors.dataTypeUnsupportedError(dt)
throw QueryExecutionErrors.dataTypeUnsupportedForWriterFuncError(dt)
Copy link
Contributor

Choose a reason for hiding this comment

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

seems we can never reach here unless we have a bug. maybe we don't need error class here and can throw IllegalStateException directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

How about to not handle the default case at all (I mean remove it). The compiler should output a warning if we forget to handle a type, and we.will get a match error exception in the case too.

Copy link
Member Author

Choose a reason for hiding this comment

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

After offline discussion, I'm going to replace the exception by IllegalStateException.

test("UNSUPPORTED_FEATURE: unsupported types (map and struct) in Literal.apply") {
def checkUnsupportedTypeInLiteral(v: Any): Unit = {
val e = intercept[SparkRuntimeException] {
Literal(v)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a user-facing API. Can we use functions.lit?

pivotVal.toString, pivotVal.dataType.simpleString, pivotCol.dataType.catalogString))
}

def unsupportedIfNotExistsError(tableName: String): Throwable = {
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we add a QueryCompilationErrorsSuite and test this error?

Copy link
Member Author

Choose a reason for hiding this comment

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

InsertIntoSQLOnlyTests has a test for the error already. BTW, the test from InsertIntoSQLOnlyTests runs as part of:

  • V1WriteFallbackSessionCatalogSuite
  • DataSourceV2SQLSuite
  • DataSourceV2SQLSessionCatalogSuite
  • DataSourceV2DataFrameSuite
  • DataSourceV2DataFrameSessionCatalogSuite

@cloud-fan How about to create the QueryCompilationErrorsSuiteBase trait/abstract class and include it to those test suites?

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 added the base test suites QueryCompilationErrorsSuiteBase + SessionCatalogTestBase and new test suites for compilation errors:

  • QueryCompilationErrorsDSv2Suite
  • QueryCompilationErrorsDSv2SessionCatalogSuite
  • QueryCompilationErrorsV1WriteFallbackSuite

messageParameters = Array(nodeName))
new SparkUnsupportedOperationException(
errorClass = "UNSUPPORTED_FEATURE",
messageParameters = Array(s"$nodeName does not implement simpleStringWithNodeId"))
Copy link
Contributor

Choose a reason for hiding this comment

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

this does not sound like a user-facing error. Can we double-check it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems, it is not. At least, I haven't found a way how to trigger it from user level. I will replace the exception by IllegalStateException as for #35302 (comment).

@MaxGekk MaxGekk requested a review from cloud-fan January 26, 2022 20:47
@MaxGekk
Copy link
Member Author

MaxGekk commented Jan 27, 2022

@cloud-fan Could you look at this one more time, please.

.agg(sum($"sales.earnings"))
.collect()
}
assert(e2.getMessage === "The feature is not supported: " +
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH, this message is quite misleading, as the query does not create literals directly. We can improve it later.

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 opened the ticket SPARK-38097 to improve the error.

@cloud-fan
Copy link
Contributor

cloud-fan commented Jan 27, 2022

thanks, merging to master!

@cloud-fan cloud-fan closed this in 88e8006 Jan 27, 2022
senthh pushed a commit to senthh/spark-1 that referenced this pull request Feb 3, 2022
…eatures by `UNSUPPORTED_FEATURE`

In the PR, I propose to re-use one error class `UNSUPPORTED_FEATURE` in the following Spark's exceptions:
- `QueryCompilationErrors.unsupportedIfNotExistsError` - when `IF NOT EXISTS` is not supported by `INSERT INTO`.
- `QueryExecutionErrors.aesModeUnsupportedError` - when an user specify unsupported AES mode and padding.
- `QueryExecutionErrors.literalTypeUnsupportedError` - impossible to create a literal from the input value (some Java class, for instance).
- `QueryExecutionErrors.transactionUnsupportedByJdbcServerError` - the target JDBC server does not support transaction.

And replace the following exceptions by `IllegalStateException` since they are internal, and should be not visible to users:
- `QueryExecutionErrors.simpleStringWithNodeIdUnsupportedError` - a sub-class of `Expression` or `Block` doesn't implements the method `simpleStringWithNodeId()`.
- `QueryExecutionErrors.dataTypeUnsupportedForWriterFuncError` - generating of a writer function for a struct field, array element, map key or map value doesn't support Catalyst's type.

Also, added new base test suite `QueryCompilationErrorsDSv2Suite` for testing DSv2 specific compilation error classes.

Reducing the number of error classes should prevent from explode of `error-classes.json`. Also, using one error class for similar errors should improve user experience with Spark SQL.

Yes.

By running the affected test suites:
```
$ build/sbt "test:testOnly *SparkThrowableSuite"
$ build/sbt "test:testOnly *QueryExecutionErrorsSuite"
$ build/sbt "test:testOnly *DataSourceV2SQLSuite"
$ build/sbt "test:testOnly *DataSourceV2DataFrameSessionCatalogSuite"
$ build/sbt "test:testOnly *DataFramePivotSuite"
```
and the new test suite:
```
$ build/sbt "test:testOnly *QueryCompilationErrorsDSv2Suite"
```

Closes apache#35302 from MaxGekk/re-use-unsupported_feature-error-class.

Authored-by: Max Gekk <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
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.

2 participants