abhishekrb19 commented on code in PR #16529:
URL: https://github.com/apache/druid/pull/16529#discussion_r1631485567
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -330,25 +329,67 @@ public static Long convertObjectToLong(@Nullable Object
valObj, boolean reportPa
} else if (valObj instanceof String) {
Long ret = DimensionHandlerUtils.getExactLongFromDecimalString((String)
valObj);
if (reportParseExceptions && ret == null) {
- throw new ParseException((String) valObj, "could not convert value
[%s] to long", valObj);
+ final String message = (objectKey != null) ?
StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long for dimension [%s]",
+ valObj,
+ objectKey
+ ) : StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long",
+ valObj
+ );
+ throw new ParseException((String) valObj, message);
}
return ret;
} else if (valObj instanceof List) {
- throw new ParseException(
- valObj.getClass().toString(),
+ final String message = (objectKey != null) ? StringUtils.nonStrictFormat(
+ "Could not ingest value [%s] as long for dimension [%s]. A long
column cannot have multiple values in the same row.",
+ valObj,
+ objectKey
+ ) : StringUtils.nonStrictFormat(
"Could not ingest value %s as long. A long column cannot have
multiple values in the same row.",
Review Comment:
While at it, could we also interpolate the value for the existing message?
```suggestion
"Could not ingest value [%s] as long. A long column cannot have
multiple values in the same row.",
```
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -330,25 +329,67 @@ public static Long convertObjectToLong(@Nullable Object
valObj, boolean reportPa
} else if (valObj instanceof String) {
Long ret = DimensionHandlerUtils.getExactLongFromDecimalString((String)
valObj);
if (reportParseExceptions && ret == null) {
- throw new ParseException((String) valObj, "could not convert value
[%s] to long", valObj);
+ final String message = (objectKey != null) ?
StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long for dimension [%s]",
Review Comment:
```suggestion
"Could not convert value [%s] to long for dimension [%s].",
```
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -330,25 +329,67 @@ public static Long convertObjectToLong(@Nullable Object
valObj, boolean reportPa
} else if (valObj instanceof String) {
Long ret = DimensionHandlerUtils.getExactLongFromDecimalString((String)
valObj);
if (reportParseExceptions && ret == null) {
- throw new ParseException((String) valObj, "could not convert value
[%s] to long", valObj);
+ final String message = (objectKey != null) ?
StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long for dimension [%s]",
+ valObj,
+ objectKey
+ ) : StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long",
+ valObj
+ );
+ throw new ParseException((String) valObj, message);
}
return ret;
} else if (valObj instanceof List) {
- throw new ParseException(
- valObj.getClass().toString(),
+ final String message = (objectKey != null) ? StringUtils.nonStrictFormat(
+ "Could not ingest value [%s] as long for dimension [%s]. A long
column cannot have multiple values in the same row.",
+ valObj,
+ objectKey
+ ) : StringUtils.nonStrictFormat(
"Could not ingest value %s as long. A long column cannot have
multiple values in the same row.",
Review Comment:
```suggestion
"Could not ingest value [%s] as long. A long column cannot have
multiple values in the same row.",
```
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -330,25 +329,67 @@ public static Long convertObjectToLong(@Nullable Object
valObj, boolean reportPa
} else if (valObj instanceof String) {
Long ret = DimensionHandlerUtils.getExactLongFromDecimalString((String)
valObj);
if (reportParseExceptions && ret == null) {
- throw new ParseException((String) valObj, "could not convert value
[%s] to long", valObj);
+ final String message = (objectKey != null) ?
StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long for dimension [%s]",
+ valObj,
+ objectKey
+ ) : StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long",
+ valObj
+ );
+ throw new ParseException((String) valObj, message);
}
return ret;
} else if (valObj instanceof List) {
- throw new ParseException(
- valObj.getClass().toString(),
+ final String message = (objectKey != null) ? StringUtils.nonStrictFormat(
+ "Could not ingest value [%s] as long for dimension [%s]. A long
column cannot have multiple values in the same row.",
+ valObj,
+ objectKey
+ ) : StringUtils.nonStrictFormat(
"Could not ingest value %s as long. A long column cannot have
multiple values in the same row.",
valObj
);
- } else {
throw new ParseException(
+ valObj.getClass().toString(),
+ message
+ );
+ } else {
+ final String message = (objectKey != null) ? StringUtils.nonStrictFormat(
+ "Could not convert value [%s] to long for dimension [%s]. Invalid
type: [%s]",
+ valObj,
+ objectKey,
+ valObj.getClass()
+ ) : StringUtils.nonStrictFormat(
valObj.getClass().toString(),
Review Comment:
Is this an existing bug - why is `valObj.getClass().toString()` the first
argument here?
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -330,25 +329,67 @@ public static Long convertObjectToLong(@Nullable Object
valObj, boolean reportPa
} else if (valObj instanceof String) {
Long ret = DimensionHandlerUtils.getExactLongFromDecimalString((String)
valObj);
if (reportParseExceptions && ret == null) {
- throw new ParseException((String) valObj, "could not convert value
[%s] to long", valObj);
+ final String message = (objectKey != null) ?
StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long for dimension [%s]",
+ valObj,
+ objectKey
+ ) : StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long",
Review Comment:
```suggestion
"Could not convert value [%s] to long.",
```
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -518,25 +619,49 @@ public static Double convertObjectToDouble(@Nullable
Object valObj, boolean repo
} else if (valObj instanceof String) {
Double ret = Doubles.tryParse((String) valObj);
if (reportParseExceptions && ret == null) {
- throw new ParseException((String) valObj, "could not convert value
[%s] to double", valObj);
+ final String message = (fieldName != null) ?
StringUtils.nonStrictFormat(
Review Comment:
Captialization and code formatting comment here and below :)
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -330,25 +329,67 @@ public static Long convertObjectToLong(@Nullable Object
valObj, boolean reportPa
} else if (valObj instanceof String) {
Long ret = DimensionHandlerUtils.getExactLongFromDecimalString((String)
valObj);
if (reportParseExceptions && ret == null) {
- throw new ParseException((String) valObj, "could not convert value
[%s] to long", valObj);
+ final String message = (objectKey != null) ?
StringUtils.nonStrictFormat(
Review Comment:
Nit: for readability, I'd suggest using a simple if else instead of a
multi-line ternary
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -330,25 +329,67 @@ public static Long convertObjectToLong(@Nullable Object
valObj, boolean reportPa
} else if (valObj instanceof String) {
Long ret = DimensionHandlerUtils.getExactLongFromDecimalString((String)
valObj);
if (reportParseExceptions && ret == null) {
- throw new ParseException((String) valObj, "could not convert value
[%s] to long", valObj);
+ final String message = (objectKey != null) ?
StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long for dimension [%s]",
+ valObj,
+ objectKey
+ ) : StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long",
+ valObj
+ );
+ throw new ParseException((String) valObj, message);
}
return ret;
} else if (valObj instanceof List) {
- throw new ParseException(
- valObj.getClass().toString(),
+ final String message = (objectKey != null) ? StringUtils.nonStrictFormat(
+ "Could not ingest value [%s] as long for dimension [%s]. A long
column cannot have multiple values in the same row.",
+ valObj,
+ objectKey
+ ) : StringUtils.nonStrictFormat(
"Could not ingest value %s as long. A long column cannot have
multiple values in the same row.",
valObj
);
- } else {
throw new ParseException(
+ valObj.getClass().toString(),
+ message
+ );
+ } else {
+ final String message = (objectKey != null) ? StringUtils.nonStrictFormat(
+ "Could not convert value [%s] to long for dimension [%s]. Invalid
type: [%s]",
+ valObj,
+ objectKey,
+ valObj.getClass()
+ ) : StringUtils.nonStrictFormat(
valObj.getClass().toString(),
"Could not convert value [%s] to long. Invalid type: [%s]",
valObj,
valObj.getClass()
);
+ throw new ParseException(
+ valObj.getClass().toString(),
+ message
+ );
}
}
+ @Nullable
+ public static Long convertObjectToLong(@Nullable Object valObj)
+ {
+ return convertObjectToLong(valObj, false);
+ }
+
+ @Nullable
+ public static Long convertObjectToLong(@Nullable Object valObj, boolean
reportParseExceptions)
+ {
+ return convertObjectToLong(valObj, reportParseExceptions, null);
+ }
+
+ @Nullable
+ public static Long convertObjectToLong(Object value, String fieldName)
Review Comment:
Looks like the first arg can be nullable similar to the other double and
float variants (also suggest renaming it to `valObj` for consistency):
```suggestion
public static Long convertObjectToLong(@Nullable Object valObj, String
fieldName)
```
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -358,58 +399,96 @@ public static Float convertObjectToFloat(@Nullable Object
valObj)
@Nullable
public static Float convertObjectToFloat(@Nullable Object valObj, boolean
reportParseExceptions)
{
- if (valObj == null) {
- return null;
- }
+ return convertObjectToFloat(valObj, reportParseExceptions, null);
+ }
- if (valObj instanceof Float) {
- return (Float) valObj;
- } else if (valObj instanceof Number) {
- return ((Number) valObj).floatValue();
- } else if (valObj instanceof String) {
- Float ret = Floats.tryParse((String) valObj);
- if (reportParseExceptions && ret == null) {
- throw new ParseException((String) valObj, "could not convert value
[%s] to float", valObj);
+ @Nullable
+ public static Float convertObjectToFloat(@Nullable Object valObj, @Nullable
String fieldName)
Review Comment:
Question on the `@Nullable` annotation: this new overloaded function appears
to be called only from `SqlResults` similar to `convertObjectToLong()` where
the fieldName doesn't have a `@Nullable` annotation. I don't think `fieldName`
can be nullable in this context, right? If that's correct, please remove the
annotation here; otherwise, update `convertObjectToLong`'s signature above to
reflect that
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -330,25 +329,67 @@ public static Long convertObjectToLong(@Nullable Object
valObj, boolean reportPa
} else if (valObj instanceof String) {
Long ret = DimensionHandlerUtils.getExactLongFromDecimalString((String)
valObj);
if (reportParseExceptions && ret == null) {
- throw new ParseException((String) valObj, "could not convert value
[%s] to long", valObj);
+ final String message = (objectKey != null) ?
StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long for dimension [%s]",
+ valObj,
+ objectKey
+ ) : StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long",
+ valObj
+ );
+ throw new ParseException((String) valObj, message);
}
return ret;
} else if (valObj instanceof List) {
- throw new ParseException(
- valObj.getClass().toString(),
+ final String message = (objectKey != null) ? StringUtils.nonStrictFormat(
+ "Could not ingest value [%s] as long for dimension [%s]. A long
column cannot have multiple values in the same row.",
+ valObj,
+ objectKey
+ ) : StringUtils.nonStrictFormat(
"Could not ingest value %s as long. A long column cannot have
multiple values in the same row.",
Review Comment:
I think it should be less confusing if all the logs here are interpolated
consistently with `[]` adhering to the [style
guide](https://github.com/apache/druid/blob/master/dev/style-conventions.md)
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -330,25 +329,67 @@ public static Long convertObjectToLong(@Nullable Object
valObj, boolean reportPa
} else if (valObj instanceof String) {
Long ret = DimensionHandlerUtils.getExactLongFromDecimalString((String)
valObj);
if (reportParseExceptions && ret == null) {
- throw new ParseException((String) valObj, "could not convert value
[%s] to long", valObj);
+ final String message = (objectKey != null) ?
StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long for dimension [%s]",
+ valObj,
+ objectKey
+ ) : StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long",
+ valObj
+ );
+ throw new ParseException((String) valObj, message);
}
return ret;
} else if (valObj instanceof List) {
- throw new ParseException(
- valObj.getClass().toString(),
+ final String message = (objectKey != null) ? StringUtils.nonStrictFormat(
+ "Could not ingest value [%s] as long for dimension [%s]. A long
column cannot have multiple values in the same row.",
+ valObj,
+ objectKey
+ ) : StringUtils.nonStrictFormat(
"Could not ingest value %s as long. A long column cannot have
multiple values in the same row.",
valObj
);
- } else {
throw new ParseException(
+ valObj.getClass().toString(),
+ message
+ );
+ } else {
+ final String message = (objectKey != null) ? StringUtils.nonStrictFormat(
+ "Could not convert value [%s] to long for dimension [%s]. Invalid
type: [%s]",
+ valObj,
+ objectKey,
+ valObj.getClass()
+ ) : StringUtils.nonStrictFormat(
valObj.getClass().toString(),
"Could not convert value [%s] to long. Invalid type: [%s]",
valObj,
valObj.getClass()
);
Review Comment:
nit: you can make this a bit concise and remove the multi-line ternary
formatting for readability:
```suggestion
final String message;
if (objectKey != null) {
message = StringUtils.nonStrictFormat(
"Could not convert value [%s] to long for dimension [%s].
Invalid type: [%s]",
valObj, objectKey, valObj.getClass()
);
} else {
message = StringUtils.nonStrictFormat(
"Could not convert value [%s] to long. Invalid type: [%s]",
valObj, valObj.getClass()
);
}
```
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -506,6 +595,18 @@ public static Double convertObjectToDouble(@Nullable
Object valObj)
@Nullable
public static Double convertObjectToDouble(@Nullable Object valObj, boolean
reportParseExceptions)
+ {
+ return convertObjectToDouble(valObj, reportParseExceptions, null);
+ }
+
+ @Nullable
+ public static Double convertObjectToDouble(@Nullable Object valObj,
@Nullable String fieldName)
Review Comment:
```suggestion
public static Double convertObjectToDouble(@Nullable Object valObj, String
fieldName)
```
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java:
##########
@@ -330,25 +329,67 @@ public static Long convertObjectToLong(@Nullable Object
valObj, boolean reportPa
} else if (valObj instanceof String) {
Long ret = DimensionHandlerUtils.getExactLongFromDecimalString((String)
valObj);
if (reportParseExceptions && ret == null) {
- throw new ParseException((String) valObj, "could not convert value
[%s] to long", valObj);
+ final String message = (objectKey != null) ?
StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long for dimension [%s]",
+ valObj,
+ objectKey
+ ) : StringUtils.nonStrictFormat(
+ "could not convert value [%s] to long",
+ valObj
+ );
+ throw new ParseException((String) valObj, message);
}
return ret;
} else if (valObj instanceof List) {
- throw new ParseException(
- valObj.getClass().toString(),
+ final String message = (objectKey != null) ? StringUtils.nonStrictFormat(
Review Comment:
Should we also include `"Invalid type: []` with `valObj.getClass()"` similar
to the other error message? (Either instead of "cannot have multiple values" or
as something that complements it to be clear)
--
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]
