cryptoe commented on code in PR #14534:
URL: https://github.com/apache/druid/pull/14534#discussion_r1261977768
##########
sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalOperatorConversion.java:
##########
@@ -90,44 +91,73 @@ public ExternalTableSpec apply(
final ObjectMapper jsonMapper
)
{
- try {
- final String sigValue = CatalogUtils.getString(args, SIGNATURE_PARAM);
- if (sigValue == null && columns == null) {
- throw new IAE(
- "EXTERN requires either a %s value or an EXTEND clause",
- SIGNATURE_PARAM
- );
+ final String sigValue = CatalogUtils.getString(args, SIGNATURE_PARAM);
+ if (sigValue == null && columns == null) {
+ throw InvalidInput.exception(
+ "EXTERN requires either a %s value or an EXTEND clause",
+ SIGNATURE_PARAM
+ );
+ }
+ if (sigValue != null && columns != null) {
+ throw InvalidInput.exception(
+ "EXTERN requires either a %s value or an EXTEND clause, but not
both",
Review Comment:
[%s] square brackets
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java:
##########
@@ -842,22 +826,23 @@ private static void throwIfQueryIsNotSuccessful(String
queryId, TaskStatusPlus s
private void contextChecks(QueryContext queryContext)
{
- ExecutionMode executionMode = queryContext.getEnum(
- QueryContexts.CTX_EXECUTION_MODE,
- ExecutionMode.class,
- null
- );
+ ExecutionMode executionMode =
queryContext.getEnum(QueryContexts.CTX_EXECUTION_MODE, ExecutionMode.class,
null);
+
+ if (executionMode == null) {
+ throw InvalidInput.exception(
+ "Execution mode is not provided to the SQL statement API. Please set
\"%s\" in the query context",
+ QueryContexts.CTX_EXECUTION_MODE
+ );
+ }
+
if (ExecutionMode.ASYNC != executionMode) {
- throw DruidException.forPersona(DruidException.Persona.USER)
- .ofCategory(DruidException.Category.INVALID_INPUT)
- .build(
- StringUtils.format(
- "The statement sql api only supports sync
mode[%s]. Please set context parameter [%s=%s] in the context payload",
- ExecutionMode.ASYNC,
- QueryContexts.CTX_EXECUTION_MODE,
- ExecutionMode.ASYNC
- )
- );
+ throw InvalidInput.exception(
+ "The SQL statement API currently does not support the provided
execution mode [%s]. "
+ + "Please set \"%s\" to [%s] in the query context",
Review Comment:
lets replace `\"%s\"` to `[%s]` everywhere
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskQueryMaker.java:
##########
@@ -228,16 +232,19 @@ public QueryResponse<Object[]> runQuery(final DruidQuery
druidQuery)
replaceTimeChunks
);
} else {
- if (ctxDestination != null &&
!DESTINATION_REPORT.equals(ctxDestination)) {
- throw new IAE("Cannot SELECT with destination [%s]", ctxDestination);
- }
final MSQSelectDestination msqSelectDestination =
MultiStageQueryContext.getSelectDestination(sqlQueryContext);
if (msqSelectDestination.equals(MSQSelectDestination.TASK_REPORT)) {
destination = TaskReportMSQDestination.instance();
} else if
(msqSelectDestination.equals(MSQSelectDestination.DURABLE_STORAGE)) {
destination = DurableStorageMSQDestination.instance();
} else {
- throw new IAE("Cannot SELECT with destination [%s]",
msqSelectDestination.name());
+ throw InvalidInput.exception(
+ "Unsupported select destination [%s] provided in the query
context. MSQ can currently write the select results to "
+ + "\"%s\" and \"%s\"",
Review Comment:
```suggestion
+ "[%s] and [%s]",
```
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskSqlEngine.java:
##########
@@ -220,13 +223,18 @@ private static void validateInsert(
);
}
catch (Exception e) {
- throw new ValidationException(
- StringUtils.format(
- "Invalid segmentGranularity: %s",
-
plannerContext.queryContext().get(DruidSqlInsert.SQL_INSERT_SEGMENT_GRANULARITY)
- ),
- e
- );
+ // This is a defensive check as the
DruidSqlInsert.SQL_INSERT_SEGMENT_GRANULARITY in the query context is
+ // populated by Druid. If the user entered an incorrect granularity,
that should have been flagged before reaching
+ // here
+ throw DruidException.forPersona(DruidException.Persona.DEVELOPER)
+ .ofCategory(DruidException.Category.DEFENSIVE)
+ .build(
+ e,
+ "Invalid %s: %s",
Review Comment:
```suggestion
"Invalid [%s]: [%s]",
```
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java:
##########
@@ -260,11 +258,7 @@ public Response doGetStatus(
}
catch (ForbiddenException e) {
log.debug("Got forbidden request for reason [%s]", e.getErrorMessage());
- return buildNonOkResponse(
- DruidException.forPersona(DruidException.Persona.USER)
- .ofCategory(DruidException.Category.FORBIDDEN)
- .build(Access.DEFAULT_ERROR_MESSAGE)
- );
+ return buildNonOkResponse(Forbidden.exception());
}
catch (Exception e) {
log.warn(e, "Failed to handle query: %s", queryId);
Review Comment:
```suggestion
log.warn(e, "Failed to handle query: [%s]", queryId);
```
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java:
##########
@@ -842,22 +826,23 @@ private static void throwIfQueryIsNotSuccessful(String
queryId, TaskStatusPlus s
private void contextChecks(QueryContext queryContext)
{
- ExecutionMode executionMode = queryContext.getEnum(
- QueryContexts.CTX_EXECUTION_MODE,
- ExecutionMode.class,
- null
- );
+ ExecutionMode executionMode =
queryContext.getEnum(QueryContexts.CTX_EXECUTION_MODE, ExecutionMode.class,
null);
+
+ if (executionMode == null) {
+ throw InvalidInput.exception(
+ "Execution mode is not provided to the SQL statement API. Please set
\"%s\" in the query context",
Review Comment:
We should also tell the user to set
`"Please set \"%s\" to [%s] in the query context",` in the query context so
he is clear of the next step to become successful.
##########
sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalOperatorConversion.java:
##########
@@ -90,44 +91,73 @@ public ExternalTableSpec apply(
final ObjectMapper jsonMapper
)
{
- try {
- final String sigValue = CatalogUtils.getString(args, SIGNATURE_PARAM);
- if (sigValue == null && columns == null) {
- throw new IAE(
- "EXTERN requires either a %s value or an EXTEND clause",
- SIGNATURE_PARAM
- );
+ final String sigValue = CatalogUtils.getString(args, SIGNATURE_PARAM);
+ if (sigValue == null && columns == null) {
+ throw InvalidInput.exception(
+ "EXTERN requires either a %s value or an EXTEND clause",
Review Comment:
[%s] square brackets
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
