imply-cheddar commented on code in PR #14534:
URL: https://github.com/apache/druid/pull/14534#discussion_r1255244519
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskQueryMaker.java:
##########
@@ -136,15 +136,24 @@ public QueryResponse<Object[]> runQuery(final DruidQuery
druidQuery)
.orElse(jsonMapper.writeValueAsString(DEFAULT_SEGMENT_GRANULARITY));
}
catch (JsonProcessingException e) {
- throw new IAE("Unable to deserialize the insert granularity. Please
retry the query with a valid "
- + "segment graularity");
+ // This isn't populated by the user in the query context. If there was
an issue with the query granularity
+ // entered by the user, it should have been flagged earlier
+ throw DruidException.forPersona(DruidException.Persona.DEVELOPER)
+ .ofCategory(DruidException.Category.DEFENSIVE)
+ .build(
+ e,
+ "Unable to deserialize the insert granularity.
Please retry the query with a "
+ + "valid segment graularity"
+ );
Review Comment:
This message reads to me like it's trying to get to the end user, but a
defensive exception is something that we expect to never happen (because the
code doesn't allow it) and therefore it faces the developer. Given this
message, are you sure that this isn't an `InvalidSqlInput`?
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskQueryMaker.java:
##########
@@ -136,15 +136,24 @@ public QueryResponse<Object[]> runQuery(final DruidQuery
druidQuery)
.orElse(jsonMapper.writeValueAsString(DEFAULT_SEGMENT_GRANULARITY));
}
catch (JsonProcessingException e) {
- throw new IAE("Unable to deserialize the insert granularity. Please
retry the query with a valid "
- + "segment graularity");
+ // This isn't populated by the user in the query context. If there was
an issue with the query granularity
+ // entered by the user, it should have been flagged earlier
+ throw DruidException.forPersona(DruidException.Persona.DEVELOPER)
+ .ofCategory(DruidException.Category.DEFENSIVE)
+ .build(
+ e,
+ "Unable to deserialize the insert granularity.
Please retry the query with a "
+ + "valid segment graularity"
+ );
}
final int maxNumTasks =
MultiStageQueryContext.getMaxNumTasks(sqlQueryContext);
if (maxNumTasks < 2) {
- throw new IAE(MultiStageQueryContext.CTX_MAX_NUM_TASKS
- + " cannot be less than 2 since at least 1 controller and
1 worker is necessary.");
+ throw InvalidInput.exception(
+ "%s cannot be less than 2 since at least 1 controller and 1 worker
is necessary.",
+ MultiStageQueryContext.CTX_MAX_NUM_TASKS
+ );
Review Comment:
This message doesn't follow our conventions for how to write error messages
or interpolate things. Please read `style-conventions.md` and adjust.
In this case, I would probably suggest
"MSQ context maxNumTasks [%,d] cannot be less than 2, since at least 1
controller and 1 worker is necessary"
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskQueryMaker.java:
##########
@@ -201,15 +210,19 @@ public QueryResponse<Object[]> runQuery(final DruidQuery
druidQuery)
if (targetDataSource != null) {
if (ctxDestination != null &&
!DESTINATION_DATASOURCE.equals(ctxDestination)) {
- throw new IAE("Cannot INSERT with destination [%s]", ctxDestination);
+ throw DruidException.forPersona(DruidException.Persona.DEVELOPER)
+ .ofCategory(DruidException.Category.DEFENSIVE)
+ .build("Cannot INSERT with destination [%s]",
ctxDestination);
Review Comment:
This message again doesn't seem defensive, it seems like it's intended for
the end-user and should be `InvalidSqlInput` no?
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskSqlEngine.java:
##########
@@ -199,12 +205,10 @@ private static void validateInsert(
// Validate the __time field has the proper type.
final SqlTypeName timeType =
rootRel.getRowType().getFieldList().get(field.left).getType().getSqlTypeName();
if (timeType != SqlTypeName.TIMESTAMP) {
- throw new ValidationException(
- StringUtils.format(
- "Field \"%s\" must be of type TIMESTAMP (was %s)",
- ColumnHolder.TIME_COLUMN_NAME,
- timeType
- )
+ throw InvalidSqlInput.exception(
+ "Field \"%s\" must be of type TIMESTAMP (was %s)",
+ ColumnHolder.TIME_COLUMN_NAME,
+ timeType
);
Review Comment:
This message doesn't follow our conventions. In this case, just make it
```
"Field [%s] was the wrong type [%s], expected TIMESTAMP"
```
##########
sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalOperatorConversion.java:
##########
@@ -106,28 +106,72 @@ public ExternalTableSpec apply(
}
final RowSignature rowSignature;
if (columns != null) {
- rowSignature = Columns.convertSignature(columns);
+ try {
+ rowSignature = Columns.convertSignature(columns);
+ }
+ catch (IAE e) {
+ throw new ArgumentAndException("columns", e);
+ }
Review Comment:
Instead of this throw/catch to just throw/catch again, why not create a
private method
```
public static InvalidInput badArgumentException(Throwable cause, String
fieldName)
{
return InvalidInput.exception(
cause,
"Invalid value for the field [%s]. Reason: [%s]",
fieldName,
e.getMessage()
);
}
```
Or, perhaps better yet would be to make the place that's throwing the IAE
actually throw the right DruidException instead. You can do a `try/catch
(DruidException e) { throw e.prependAndBuild("Problem converting columns") }`
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskQueryMaker.java:
##########
@@ -136,15 +136,24 @@ public QueryResponse<Object[]> runQuery(final DruidQuery
druidQuery)
.orElse(jsonMapper.writeValueAsString(DEFAULT_SEGMENT_GRANULARITY));
}
catch (JsonProcessingException e) {
- throw new IAE("Unable to deserialize the insert granularity. Please
retry the query with a valid "
- + "segment graularity");
+ // This isn't populated by the user in the query context. If there was
an issue with the query granularity
+ // entered by the user, it should have been flagged earlier
+ throw DruidException.forPersona(DruidException.Persona.DEVELOPER)
Review Comment:
You can just do `DruidException.defensive()` now.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskQueryMaker.java:
##########
@@ -227,7 +240,9 @@ public QueryResponse<Object[]> runQuery(final DruidQuery
druidQuery)
);
} else {
if (ctxDestination != null &&
!DESTINATION_REPORT.equals(ctxDestination)) {
- throw new IAE("Cannot SELECT with destination [%s]", ctxDestination);
+ throw DruidException.forPersona(DruidException.Persona.DEVELOPER)
+ .ofCategory(DruidException.Category.DEFENSIVE)
+ .build("Cannot SELECT with destination [%s]",
ctxDestination);
Review Comment:
What's wrong with the destination such that you cannot select with it? This
message claims to be defensive, but it feels a lot like it's user-facing and I
have no clue what action I'm supposed to take?
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskQueryMaker.java:
##########
@@ -201,15 +210,19 @@ public QueryResponse<Object[]> runQuery(final DruidQuery
druidQuery)
if (targetDataSource != null) {
if (ctxDestination != null &&
!DESTINATION_DATASOURCE.equals(ctxDestination)) {
- throw new IAE("Cannot INSERT with destination [%s]", ctxDestination);
+ throw DruidException.forPersona(DruidException.Persona.DEVELOPER)
+ .ofCategory(DruidException.Category.DEFENSIVE)
+ .build("Cannot INSERT with destination [%s]",
ctxDestination);
}
Granularity segmentGranularityObject;
try {
segmentGranularityObject = jsonMapper.readValue((String)
segmentGranularity, Granularity.class);
}
catch (Exception e) {
- throw new ISE("Unable to convert %s to a segment granularity",
segmentGranularity);
+ throw DruidException.forPersona(DruidException.Persona.DEVELOPER)
+ .ofCategory(DruidException.Category.DEFENSIVE)
+ .build("Unable to convert %s to a segment
granularity", segmentGranularity);
Review Comment:
This is throwing away the `Exception e` so we will never be able to figure
out why we were unable to convert the value. It also doesn't follow our
conventions for how to write and interpolate messages and I'm not sure that
it's truly defensive?
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskSqlEngine.java:
##########
@@ -237,11 +246,9 @@ private static void validateInsert(
validateLimitAndOffset(rootRel, !hasSegmentGranularity);
if (hasSegmentGranularity && timeFieldIndex < 0) {
- throw new ValidationException(
- StringUtils.format(
- "INSERT queries with segment granularity other than \"all\" must
have a \"%s\" field.",
- ColumnHolder.TIME_COLUMN_NAME
- )
+ throw InvalidInput.exception(
+ "INSERT queries with segment granularity other than \"all\" must
have a \"%s\" field.",
+ ColumnHolder.TIME_COLUMN_NAME
);
Review Comment:
This message doesn't follow our conventions for error messages.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskSqlEngine.java:
##########
@@ -125,17 +125,20 @@ public boolean featureAvailable(EngineFeature feature,
PlannerContext plannerCon
case SCAN_NEEDS_SIGNATURE:
return true;
default:
- throw new IAE("Unrecognized feature: %s", feature);
+ throw DruidException
+ .forPersona(DruidException.Persona.DEVELOPER)
+ .ofCategory(DruidException.Category.DEFENSIVE)
+ .build("Unrecognized feature: %s", feature);
Review Comment:
This doesn't seem defensive either? This message seems intended for the
admin or the operator perhaps? That or it could be end user, do we document
these "features" or are they intended to be hidden?
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskSqlEngine.java:
##########
@@ -220,13 +224,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
Review Comment:
You can put all of this into the message instead of a comment. That way, if
this ever does get fired, the person who sees it will know without needing to
find this comment in the code. Also, if it's in the message, you don't need a
comment in the code.
For example, you could say "Unable to parse the segmentGranularity [%s].
This should have been created by class XYZ, so this exception most likely
indicates a bug over there."
Given that this is a developer-facing message, it's perfectly fine to
include such details about the code and expectations of the code.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskSqlEngine.java:
##########
@@ -169,23 +172,26 @@ public QueryMaker buildQueryMakerForInsert(
);
}
- private static void validateSelect(
- final List<Pair<Integer, String>> fieldMappings,
- final PlannerContext plannerContext
- ) throws ValidationException
+ /**
+ * Checks if the SELECT contains {@link
DruidSqlInsert#SQL_INSERT_SEGMENT_GRANULARITY} in the context. This is a
+ * defensive cheeck because {@link
org.apache.druid.sql.calcite.planner.DruidPlanner} should have called the
+ * {@link #validateContext}
+ */
+ private static void validateSelect(final PlannerContext plannerContext)
{
if
(plannerContext.queryContext().containsKey(DruidSqlInsert.SQL_INSERT_SEGMENT_GRANULARITY))
{
- throw new ValidationException(
- StringUtils.format("Cannot use \"%s\" without INSERT",
DruidSqlInsert.SQL_INSERT_SEGMENT_GRANULARITY)
- );
+ throw DruidException
+ .forPersona(DruidException.Persona.DEVELOPER)
+ .ofCategory(DruidException.Category.DEFENSIVE)
+ .build("Cannot use \"%s\" without INSERT",
DruidSqlInsert.SQL_INSERT_SEGMENT_GRANULARITY);
Review Comment:
Once again, I think this is an `InvalidSqlException`, not defensive. Also,
the message doens't follow our conventions.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java:
##########
@@ -150,16 +152,12 @@ public Response doPost(final SqlQuery sqlQuery, @Context
final HttpServletReques
);
if (ExecutionMode.ASYNC != executionMode) {
return buildNonOkResponse(
- 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
- )
- )
+ InvalidInput.exception(
+ "The statement sql api only supports sync mode[%s]. Please set
context parameter [%s=%s] in the context payload",
Review Comment:
I'm not sure I know what the `statement sql api` is? If it's only supposed
to be one thing and this code knows what that one thing is and the user got
their request to this line of the code, why does the user need to change their
query to only do the one thing that this code can do? I.e. why not just ignore
the context parameter entirely? Or overwrite it with the one value that this
code can do?
##########
processing/src/main/java/org/apache/druid/error/InvalidInput.java:
##########
@@ -35,7 +35,7 @@ public static DruidException exception(Throwable t, String
msg, Object... args)
private final String msg;
private final Object[] args;
- public InvalidInput(
+ protected InvalidInput(
Review Comment:
Why this change? Please back it out. `protected` pretty much doesn't help
anybody, might as well be public.
--
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]
