Jackie-Jiang commented on code in PR #18977:
URL: https://github.com/apache/pinot/pull/18977#discussion_r3779307956
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/BaseSegmentCreator.java:
##########
@@ -694,6 +707,35 @@ public static void
addColumnMetadataInfo(PropertiesConfiguration properties, Str
}
}
+ /// Records the transform function used to generate the given column in the
segment metadata properties.
+ public static void addTransformFunction(PropertiesConfiguration properties,
String column,
+ @Nullable String transformFunction) {
+ if (transformFunction != null) {
+ String validTransformFunction =
+
CommonsConfigurationUtils.replaceSpecialCharacterInPropertyValue(transformFunction);
+ if (validTransformFunction != null) {
+ properties.setProperty(getKeyFor(column, TRANSFORM_FUNCTION),
validTransformFunction);
+ }
+ }
+ }
+
+ @Nullable
+ @SuppressWarnings("deprecation")
+ private String getTransformFunctionForColumn(String column) {
Review Comment:
(minor) Instead of looping over the list multiple times (once per column),
we can first build a map from column to transform function
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/ColumnMetadata.java:
##########
@@ -57,6 +57,12 @@ default boolean isNonNull() {
/// Returns `true` when the column is auto-generated by the default column
handler.
boolean isAutoGenerated();
+ /// Returns the transform function expression used to generate the column,
if persisted in the segment metadata.
+ @Nullable
+ default String getTransformFunction() {
Review Comment:
No need to provide default impl. This is an internal interface
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:
##########
@@ -381,73 +428,83 @@ 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;
}
+ @Nullable
+ @SuppressWarnings("deprecation")
+ private String getTransformFunctionForColumn(String column) {
Review Comment:
Same here. We can create a map from column to transform functions. Suggest
extracting it into a util method to be shared
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java:
##########
@@ -847,7 +875,8 @@ public ColumnMetadataImpl build() {
return new ColumnMetadataImpl(_fieldSpec, _totalDocs, _cardinality,
_hasDictionary, _forwardIndexEncoding,
_sorted, _nonNull, _minValue, _maxValue, _minMaxValueInvalid,
_lengthOfShortestElement,
_lengthOfLongestElement, _isAscii, _totalNumberOfEntries,
_maxNumberOfMultiValues, _maxRowLengthInBytes,
- _bitsPerElement, _partitionFunction, _partitions, _autoGenerated,
_parentColumn, _sparseKeys,
+ _bitsPerElement, _partitionFunction, _partitions, _autoGenerated,
_transformFunction, _parentColumn,
+ _sparseKeys,
Review Comment:
(minor) Reformat
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/ColumnMetadataImpl.java:
##########
@@ -481,6 +494,15 @@ private static ChunkCompressionType
parseCompressionType(String column, @Nullabl
}
}
+ @Nullable
+ private static String extractTransformFunction(String column,
PropertiesConfiguration config) {
+ Object transformFunctionProperty =
config.getProperty(Column.getKeyFor(column, Column.TRANSFORM_FUNCTION));
Review Comment:
Should we use `getString()`?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:
##########
@@ -111,20 +112,28 @@ protected enum DefaultColumnAction {
UPDATE_DIMENSION_DATA_TYPE,
UPDATE_DIMENSION_DEFAULT_VALUE,
UPDATE_DIMENSION_NUMBER_OF_VALUES,
+ UPDATE_DIMENSION_TRANSFORM_FUNCTION,
UPDATE_METRIC_DATA_TYPE,
UPDATE_METRIC_DEFAULT_VALUE,
UPDATE_METRIC_NUMBER_OF_VALUES,
+ UPDATE_METRIC_TRANSFORM_FUNCTION,
UPDATE_DATE_TIME_DATA_TYPE,
UPDATE_DATE_TIME_DEFAULT_VALUE,
+ UPDATE_DATE_TIME_TRANSFORM_FUNCTION,
UPDATE_COMPLEX_DATA_TYPE,
- UPDATE_COMPLEX_DEFAULT_VALUE;
+ UPDATE_COMPLEX_DEFAULT_VALUE,
+ UPDATE_COMPLEX_TRANSFORM_FUNCTION,
+ // Metadata-only action: record the configured transform function for an
auto-generated column created before the
+ // transform function was tracked in the segment metadata. No values are
regenerated, and it is handled entirely
+ // within updateDefaultColumns(), i.e. it is never dispatched to
updateDefaultColumn().
+ BACKFILL_TRANSFORM_FUNCTION;
Review Comment:
When backfilling the transform function, we might want to fill a different
field so that we can differentiate the case of backward compatible handling vs
actual backfilled value. Currently there is no way to differentiate them
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/BaseSegmentCreator.java:
##########
@@ -619,6 +622,15 @@ protected void writeMetadata()
public static void addColumnMetadataInfo(PropertiesConfiguration properties,
String column,
ColumnStatistics columnStatistics, int totalDocs, FieldSpec fieldSpec,
boolean hasDictionary,
int dictionaryElementSize, FieldConfig.EncodingType
forwardIndexEncoding, boolean autoGenerated) {
+ addColumnMetadataInfo(properties, column, columnStatistics, totalDocs,
fieldSpec, hasDictionary,
+ dictionaryElementSize, forwardIndexEncoding, autoGenerated, null);
+ }
+
+ /// Adds column metadata information to the properties configuration.
+ public static void addColumnMetadataInfo(PropertiesConfiguration properties,
String column,
Review Comment:
(minor) Remove the old method, and change the callers to adapt to the new API
--
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]
