yashmayya commented on code in PR #13212:
URL: https://github.com/apache/pinot/pull/13212#discussion_r1682109635
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:
##########
@@ -575,44 +587,59 @@ private void createDerivedColumnV1Indices(String column,
FunctionEvaluator funct
for (int j = 0; j < numArguments; j++) {
inputValues[j] = valueReaders.get(j).getValue(i);
}
- Object outputValue = functionEvaluator.evaluate(inputValues);
- outputValues[i] = outputValue;
- if (outputValueType == null) {
+
+ Object outputValue = null;
+ try {
+ outputValue = functionEvaluator.evaluate(inputValues);
+ } catch (Exception e) {
+ if (!errorOnFailure) {
+ LOGGER.debug("Encountered an exception while evaluating function
{} for derived column {} with "
+ + "arguments: {}", functionEvaluator, column,
Arrays.toString(inputValues), e);
+ functionEvaluateErrorCount++;
+ if (functionEvalError == null) {
+ functionEvalError = e;
+ inputValuesWithError = Arrays.copyOf(inputValues,
inputValues.length);
+ }
+ } else {
+ throw e;
+ }
+ }
+
+ if (outputValue == null) {
+ outputValue = fieldSpec.getDefaultNullValue();
Review Comment:
I think it's better to handle it here in a single place rather than adding
`null` handling in multiple branches for each type? Any particular reason for
doing it that way instead?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:
##########
@@ -812,6 +775,274 @@ private void createDerivedColumnV1Indices(String column,
FunctionEvaluator funct
}
}
+ /**
+ * Helper method to convert the output of a transform function to the
appropriate type for an SV or MV
+ * {@link FieldSpec.DataType#INT} field
+ *
+ * @param outputValue the output of the transform function
+ * @param isSingleValue true if the field (column) is single-valued
+ * @param outputValueType the output value type for the transform function;
can be null (in which case,
+ * the {@code outputValue} should be the field's
default null value)
+ * @param defaultNullValue the default null value for the field
+ * @param dictionary true if the column has a dictionary. For an MV field,
this results in a primitive array being
+ * returned rather than an array of the primitive wrapper
class
+ * @return the converted output value (either an Integer, an Integer[] or an
int[])
+ */
+ private Object getIntOutputValue(Object outputValue, boolean isSingleValue,
PinotDataType outputValueType,
+ Integer defaultNullValue, boolean dictionary) {
Review Comment:
The default null value obtained from the field spec will be of the primitive
object wrapper class type, and the value that we're returning here is also an
`Object` since that's the type for the `outputValues` array. Won't using a
primitive `int` here simply result in a redundant additional autoboxing and
unboxing?
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
