ihuzenko commented on a change in pull request #1648: DRILL-7049 return
VARBINARY as a string with escaped non printable bytes
URL: https://github.com/apache/drill/pull/1648#discussion_r259788658
##########
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/util/ValueVectorElementFormatter.java
##########
@@ -71,6 +71,9 @@ public String format(Object value, TypeProtos.MinorType
minorType) {
options.getString(ExecConstants.WEB_DISPLAY_FORMAT_TIME),
(v, p) -> v.format(getTimeFormatter(p)));
}
+ case VARBINARY:
+ byte[] bytes = (byte[]) value;
Review comment:
This logic inside ```case VARBINARY:``` may cause problems. The reason is
that whole switch case is suspiciously implemented, check previous case
statements :
```java
case TIMESTAMP:
if (value instanceof LocalDateTime) {
return format((LocalDateTime) value,
options.getString(ExecConstants.WEB_DISPLAY_FORMAT_TIMESTAMP),
(v, p) -> v.format(getTimestampFormatter(p)));
}
case DATE:
if (value instanceof LocalDate) {
return format((LocalDate) value,
options.getString(ExecConstants.WEB_DISPLAY_FORMAT_DATE),
(v, p) -> v.format(getDateFormatter(p)));
}
case TIME:
if (value instanceof LocalTime) {
return format((LocalTime) value,
options.getString(ExecConstants.WEB_DISPLAY_FORMAT_TIME),
(v, p) -> v.format(getTimeFormatter(p)));
}
```
Here there is no ```break``` keyword after any ```case``` match, so if for
example in ```switch (minorType)``` we got TIMESTAMP, but ```value instanceof
LocalDateTime``` returned false, we will fall through all other ```case```
checks and code within each next ```case``` section will be called. Despite of
this all previous cases won't fail (because each one contains instance of
check), but yours will cast to ```byte[]``` immediately and may fail here. In
my opinion it would be better to rewrite the switch case by adding ```break```s
and also ensure that for ```VARBINARY``` casting to ```byte[]``` is always
safe.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
