Vamsi-klu commented on code in PR #18977:
URL: https://github.com/apache/pinot/pull/18977#discussion_r3994777283
##########
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:
Done. BACKFILL now writes the configured expression only to
`transformFunctionBackfilled`. `transformFunction` is reserved for expressions
that actually produced the stored values.
Change detection uses the stored transform if present, otherwise the
backfilled one, so a later config change still regenerates exactly once.
Readers also accept the earlier boolean `"true"` marker from the Aug 15
commit on this branch. In that case the companion `transformFunction` value is
treated as the compat expression, not a real stored transform.
Tests cover backfill vs stored so they cannot be confused, the boolean
marker still detecting a later change, and a real transform change still
rebuilding values.
##########
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:
I tried `getString()` first. Commons Configuration interpolates `${...}`,
and a Groovy transform can contain that. I added a test where `${x}` was
rewritten to another metadata key's value.
So the expression itself is still read with `getProperty()`, same reason
min/max avoid `getString()`. The boolean backfill marker is read with
`getString()` because it is just `"true"`.
##########
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:
Removed in `ec240375`. Callers now use the transformFunction overload,
including OpenStruct and the column metadata tests.
##########
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:
Done in `ec240375`. `BaseSegmentCreator` builds the column-to-transform map
once via `IngestionConfigUtils.getTransformFunctionByColumn`.
##########
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:
Done in `ec240375`. Both `BaseDefaultColumnHandler` and `BaseSegmentCreator`
use `IngestionConfigUtils.getTransformFunctionByColumn` so the list is scanned
once.
##########
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:
Removed in `ec240375`. `ColumnMetadata.getTransformFunction()` is abstract
now, with implementations on `ColumnMetadataImpl`, `EmptyColumnMetadata`, and
`SimpleColumnMetadata`. Same for `getTransformFunctionBackfilled()`.
##########
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:
Reformatted when the backfilled field was added to the constructor and
builder.
--
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]
