LakshSingla commented on code in PR #14483:
URL: https://github.com/apache/druid/pull/14483#discussion_r1240576953
##########
sql/src/test/java/org/apache/druid/sql/calcite/run/SqlResultsTest.java:
##########
@@ -102,6 +104,20 @@ public void testCoerceNestedArrays()
assertCoerced(nestedList, nestedArray, true);
}
+ @Test
+ public void testCoerceOfArrayOfPrimitives()
+ {
+ try {
+ ObjectMapper mapper = new ObjectMapper();
Review Comment:
nit: I think we should initialize this outside the test case since it would
assist any other tests trying to use an object mapper.
##########
sql/src/main/java/org/apache/druid/sql/calcite/run/SqlResults.java:
##########
@@ -161,16 +163,32 @@ public static Object coerce(
// here if needed
coercedValue = maybeCoerceArrayToList(value, true);
if (coercedValue == null) {
- throw new ISE("Cannot coerce [%s] to %s",
value.getClass().getName(), sqlTypeName);
+ throw new ISE(prepareCoerceException(value, sqlTypeName, fieldName));
}
}
} else {
- throw new ISE("Cannot coerce [%s] to %s", value.getClass().getName(),
sqlTypeName);
+ throw new ISE(prepareCoerceException(value, sqlTypeName, fieldName));
}
return coercedValue;
}
+ private static String prepareCoerceException(Object value, SqlTypeName
sqlTypeName, String fieldName)
+ {
+ return StringUtils.format(
+ "Cannot coerce field [%s] of value with class [%s] to %s",
+ fieldName,
+ value.getClass().getName(),
+ sqlTypeName
+ );
+ }
+
+ private static ISE coerceExceptionWithSupressed(Object value, SqlTypeName
sqlTypeName, String fieldName, Throwable t)
+ {
+ ISE e = new ISE(prepareCoerceException(value, sqlTypeName, fieldName));
+ e.addSuppressed(t);
Review Comment:
I am not very clear with the semantics of adding `t` as suppressed v/s
adding it as a cause.
Since t is directly causing the coercion exception that we are raising, I
suppose it should be a cause. right?
##########
sql/src/test/java/org/apache/druid/sql/calcite/run/SqlResultsTest.java:
##########
@@ -102,6 +104,20 @@ public void testCoerceNestedArrays()
assertCoerced(nestedList, nestedArray, true);
}
+ @Test
+ public void testCoerceOfArrayOfPrimitives()
+ {
+ try {
+ ObjectMapper mapper = new ObjectMapper();
Review Comment:
```suggestion
ObjectMapper mapper = new DefaultObjectMapper();
```
While it might not affect this use case, `DefaultObjectMapper()` is
preferred since it contains Druid's modules having custom serdes.
##########
sql/src/main/java/org/apache/druid/sql/calcite/run/SqlResults.java:
##########
@@ -161,16 +163,32 @@ public static Object coerce(
// here if needed
coercedValue = maybeCoerceArrayToList(value, true);
if (coercedValue == null) {
- throw new ISE("Cannot coerce [%s] to %s",
value.getClass().getName(), sqlTypeName);
+ throw new ISE(prepareCoerceException(value, sqlTypeName, fieldName));
}
}
} else {
- throw new ISE("Cannot coerce [%s] to %s", value.getClass().getName(),
sqlTypeName);
+ throw new ISE(prepareCoerceException(value, sqlTypeName, fieldName));
}
return coercedValue;
}
+ private static String prepareCoerceException(Object value, SqlTypeName
sqlTypeName, String fieldName)
+ {
+ return StringUtils.format(
+ "Cannot coerce field [%s] of value with class [%s] to %s",
+ fieldName,
+ value.getClass().getName(),
+ sqlTypeName
+ );
+ }
+
+ private static ISE coerceExceptionWithSupressed(Object value, SqlTypeName
sqlTypeName, String fieldName, Throwable t)
+ {
+ ISE e = new ISE(prepareCoerceException(value, sqlTypeName, fieldName));
Review Comment:
We should use the newly added `DruidException` class since it's a
using-facing message.
##########
sql/src/main/java/org/apache/druid/sql/calcite/run/SqlResults.java:
##########
@@ -161,16 +163,32 @@ public static Object coerce(
// here if needed
coercedValue = maybeCoerceArrayToList(value, true);
if (coercedValue == null) {
- throw new ISE("Cannot coerce [%s] to %s",
value.getClass().getName(), sqlTypeName);
+ throw new ISE(prepareCoerceException(value, sqlTypeName, fieldName));
}
}
} else {
- throw new ISE("Cannot coerce [%s] to %s", value.getClass().getName(),
sqlTypeName);
+ throw new ISE(prepareCoerceException(value, sqlTypeName, fieldName));
}
return coercedValue;
}
+ private static String prepareCoerceException(Object value, SqlTypeName
sqlTypeName, String fieldName)
+ {
+ return StringUtils.format(
+ "Cannot coerce field [%s] of value with class [%s] to %s",
Review Comment:
```suggestion
"Cannot coerce field [%s] of value with class [%s] to [%s]",
```
--
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]
