Copilot commented on code in PR #18977:
URL: https://github.com/apache/pinot/pull/18977#discussion_r3717351473
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:
##########
@@ -390,65 +409,74 @@ protected void removeColumnIndices(String column) {
protected boolean createColumnV1Indices(String column)
throws Exception {
boolean errorOnFailure = _indexLoadingConfig.isErrorOnColumnBuildFailure();
- IngestionConfig ingestionConfig = _tableConfig.getIngestionConfig();
- if (ingestionConfig != null && ingestionConfig.getTransformConfigs() !=
null) {
- List<TransformConfig> transformConfigs =
ingestionConfig.getTransformConfigs();
- for (TransformConfig transformConfig : transformConfigs) {
- if (transformConfig.getColumnName().equals(column)) {
- String transformFunction = transformConfig.getTransformFunction();
- FunctionEvaluator functionEvaluator =
FunctionEvaluatorFactory.getExpressionEvaluator(transformFunction);
-
- // Check if all arguments exist in the segment
- // TODO: Support chained derived column
- List<String> arguments = functionEvaluator.getArguments();
- List<ColumnMetadata> argumentsMetadata = new
ArrayList<>(arguments.size());
- for (String argument : arguments) {
- ColumnMetadata columnMetadata =
_segmentMetadata.getColumnMetadataFor(argument);
- if (columnMetadata == null) {
- LOGGER.warn("Assigning default value to derived column: {}
because argument: {} does not exist in the "
- + "segment", column, argument);
- createDefaultValueColumnV1Indices(column);
- return true;
- }
- // TODO: Support creation of derived columns from forward index
disabled columns
- if (!_segmentWriter.hasIndexFor(argument,
StandardIndexes.forward())) {
- throw new UnsupportedOperationException(String.format("Operation
not supported! Cannot create a derived "
- + "column %s because argument: %s does not have a
forward index. Enable forward index and "
- + "refresh/backfill the segments to create a derived
column from source column", column,
- argument));
- }
- argumentsMetadata.add(columnMetadata);
- }
+ String transformFunction = getTransformFunctionForColumn(column);
+ if (transformFunction != null) {
+ FunctionEvaluator functionEvaluator =
FunctionEvaluatorFactory.getExpressionEvaluator(transformFunction);
+
+ // Check if all arguments exist in the segment
+ // TODO: Support chained derived column
+ List<String> arguments = functionEvaluator.getArguments();
+ List<ColumnMetadata> argumentsMetadata = new
ArrayList<>(arguments.size());
+ for (String argument : arguments) {
+ ColumnMetadata columnMetadata =
_segmentMetadata.getColumnMetadataFor(argument);
+ if (columnMetadata == null) {
+ LOGGER.warn("Assigning default value to derived column: {} because
argument: {} does not exist in the "
+ + "segment", column, argument);
+ createDefaultValueColumnV1Indices(column, transformFunction);
+ return true;
+ }
+ // TODO: Support creation of derived columns from forward index
disabled columns
+ if (!_segmentWriter.hasIndexFor(argument, StandardIndexes.forward())) {
+ throw new UnsupportedOperationException(String.format("Operation not
supported! Cannot create a derived "
+ + "column %s because argument: %s does not have a forward
index. Enable forward index and "
+ + "refresh/backfill the segments to create a derived column
from source column", column,
+ argument));
+ }
+ argumentsMetadata.add(columnMetadata);
+ }
- // TODO: Support forward index disabled derived column
- if (isForwardIndexDisabled(column)) {
- LOGGER.warn("Skip creating forward index disabled derived column:
{}", column);
- if (errorOnFailure) {
- throw new UnsupportedOperationException(
- String.format("Failed to create forward index disabled
derived column: %s", column));
- }
- return false;
- }
+ // TODO: Support forward index disabled derived column
+ if (isForwardIndexDisabled(column)) {
+ LOGGER.warn("Skip creating forward index disabled derived column: {}",
column);
+ if (errorOnFailure) {
+ throw new UnsupportedOperationException(
+ String.format("Failed to create forward index disabled derived
column: %s", column));
+ }
+ return false;
+ }
- try {
- createDerivedColumnV1Indices(column, functionEvaluator,
argumentsMetadata, errorOnFailure);
- return true;
- } catch (Exception e) {
- LOGGER.error("Caught exception while creating derived column: {}
with transform function: {}", column,
- transformFunction, e);
- if (errorOnFailure) {
- throw e;
- }
- return false;
- }
+ try {
+ createDerivedColumnV1Indices(column, transformFunction,
functionEvaluator, argumentsMetadata, errorOnFailure);
+ return true;
+ } catch (Exception e) {
+ LOGGER.error("Caught exception while creating derived column: {} with
transform function: {}", column,
+ transformFunction, e);
+ if (errorOnFailure) {
+ throw e;
}
+ return false;
}
}
- createDefaultValueColumnV1Indices(column);
+ createDefaultValueColumnV1Indices(column, null);
return true;
}
+ @SuppressWarnings("deprecation")
+ private String getTransformFunctionForColumn(String column) {
+ IngestionConfig ingestionConfig = _tableConfig.getIngestionConfig();
+ if (ingestionConfig != null && ingestionConfig.getTransformConfigs() !=
null) {
+ for (TransformConfig transformConfig :
ingestionConfig.getTransformConfigs()) {
+ if (column.equals(transformConfig.getColumnName())) {
+ return transformConfig.getTransformFunction();
+ }
+ }
+ }
+ FieldSpec fieldSpec = _schema.getFieldSpecFor(column);
+ // Keep the schema-level transform fallback for legacy configs.
+ return fieldSpec != null ? fieldSpec.getTransformFunction() : null;
+ }
Review Comment:
`getTransformFunctionForColumn()` can return null (e.g., when there is no
ingestion transform config and no schema-level transform), but the method is
not annotated as nullable. Pinot code is non-null by default unless annotated,
so this makes the null contract unclear and can break nullness/static analysis.
--
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]
