Vamsi-klu commented on code in PR #18977:
URL: https://github.com/apache/pinot/pull/18977#discussion_r3755549677
##########
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:
Added in `1650595`, `getTransformFunctionForColumn` is annotated `@Nullable`
now, matching the `BaseSegmentCreator` copy.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:
##########
@@ -359,6 +373,11 @@ Map<String, DefaultColumnAction>
computeDefaultColumnActionMap() {
return defaultColumnActionMap;
}
+ private boolean isTransformFunctionChanged(String column, ColumnMetadata
columnMetadata) {
+ String transformFunctionInMetadata = columnMetadata.getTransformFunction();
+ return !Objects.equals(transformFunctionInMetadata,
getTransformFunctionForColumn(column));
Review Comment:
Good point, and I agree. A guaranteed fleet-wide rebuild on the first reload
after an upgrade is a real operational cost, and for a legacy segment we cannot
even tell whether the values are actually stale.
Changed in `1650595`: a null transform function in the metadata is no longer
treated as a change. Instead, when the stored transform is null, the column is
auto generated, and the table config declares a transform, the configured
transform is backfilled into the segment metadata through the normal reload
persistence path, with no value regeneration. So the upgrade is a no-op and the
next real transform change is detected and rebuilt exactly once.
The tradeoff, which I put in a code comment rather than leaving implicit: a
legacy segment that is already stale under an older transform stays as is until
the transform changes again, which matches today's master behavior. An operator
who wants an immediate rebuild can force one by touching the expression.
Tests cover the legacy null-metadata case producing no rebuild plus the
backfill, a second reload after a real transform change firing the update
action, and the transform-removed-from-config case.
--
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]
