HeartSaVioR commented on code in PR #47327:
URL: https://github.com/apache/spark/pull/47327#discussion_r1683979794
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala:
##########
@@ -1750,11 +1750,10 @@ private[sql] object QueryExecutionErrors extends
QueryErrorsBase with ExecutionE
}
def invalidStreamingOutputModeError(
Review Comment:
I think this exception isn't generalized enough yet.
This now does not happen at all (since _LEGACY_ERROR_TEMP_3261 has to happen
first) so it wasn't an issue, but if we deduce the meaningful error class from
this, this should probably need to couple with operator, and provide the
operator information as well, something along the line with this `"<operator
Name> does not support <outputMode> output mode."`
Here, `<operator name>` has to be user friendly, e.g. "Streaming
aggregation", not StateStoreSaveExec or so. Same for "Streaming session window
aggregation", etc.
After that, it could be bound to
INVALID_STREAMING_OUTPUT_MODE.UNSUPPORTED_OPERATOR. Conceptually it has to be
flipped, but if we really want to group these error classes into
INVALID_STREAMING_OUTPUT_MODE. If we flip,
STREAMING_OPERATOR_UNSUPPORTED_OUTPUT_MODE.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -1594,13 +1596,11 @@ private[sql] object QueryCompilationErrors extends
QueryErrorsBase with Compilat
messageParameters = Map("path" -> path))
}
- def dataSourceOutputModeUnsupportedError(
- className: String, outputMode: OutputMode): Throwable = {
+ def fileSinkOutputModeUnsupportedError(
Review Comment:
Shall we just leave the error class as same to cover arbitrary data source?
Users are more friendly to the sink format they provide, e.g. parquet, etc.
Also it can be used from any future data source if we leave flexibility.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -1594,13 +1596,11 @@ private[sql] object QueryCompilationErrors extends
QueryErrorsBase with Compilat
messageParameters = Map("path" -> path))
}
- def dataSourceOutputModeUnsupportedError(
- className: String, outputMode: OutputMode): Throwable = {
+ def fileSinkOutputModeUnsupportedError(
Review Comment:
If it's not weird to have
INVALID_STREAMING_OUTPUT_MODE.UNSUPPORTED_OPERATOR, this can be
INVALID_STREAMING_OUTPUT_MODE.UNSUPPORTED_DATA_SOURCE. But again, this has to
be flipped, like `DATA_SOURCE_UNSUPPORTED_STREAMING_OUTPUT_MODE`. (If there is
a general error class group for data source, include there.)
Probably it might not be good to group them with INVALID_STREAMING_OUTPUT.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
