You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Vamsi-klu (via GitHub)" <gi...@apache.org> on 2026/07/11 21:39:39 UTC
[PR] Regenerate derived columns when transforms change [pinot]
Vamsi-klu opened a new pull request, #18977:
URL: https://github.com/apache/pinot/pull/18977
Fixes #9989
## What changed
This PR persists derived-column transform function metadata into segment column metadata and uses that metadata during default-column reload decisions.
The implementation adds a `transformFunction` column metadata property and exposes it through `ColumnMetadata`. Segment generation now records the transform function associated with a generated column, and default-column creation records the transform function for auto-generated derived columns as well.
During reload, `BaseDefaultColumnHandler` now compares the transform function stored in segment metadata with the transform function currently configured for the column. If the value changed, or if a legacy auto-generated derived column has no stored transform metadata but the table config now declares a transform, Pinot regenerates the column instead of leaving stale derived values in the segment.
The default-value fallback path is covered as well: if a derived column's transform argument is missing from the segment and Pinot has to materialize the default value instead of evaluated values, the transform function is still persisted in metadata. That prevents the segment from repeatedly looking like it has no transform metadata on subsequent reloads.
## Why it matters
Derived columns are produced from transform expressions. If a table config changes the expression, reloaded segments need to reflect the new expression for auto-generated derived columns. Without tracking the expression in segment metadata, Pinot can know that the column exists, but not whether it was created from the current transform function.
That creates a correctness risk: a segment can continue serving values computed by an older transform even after the table config has been updated. Persisting and comparing the transform function gives reload a stable, segment-local way to decide whether an auto-generated derived column must be rebuilt.
The legacy metadata handling matters because existing segments created before this metadata existed will naturally have no transform function recorded. Those segments should not be permanently stuck with stale values once they are reloaded under a table config that defines a transform for the auto-generated column.
## Impact
Auto-generated derived columns are regenerated when their transform function changes.
Legacy auto-generated derived columns that do not yet have stored transform metadata are brought forward on reload and receive the current transform metadata.
Segment-generation metadata now includes transform-function provenance for generated columns, which improves observability and future reload decisions.
The metadata API remains backward compatible: `ColumnMetadata#getTransformFunction()` defaults to `null`, and older segment metadata without the property still loads successfully.
## Testing
- `./mvnw -Dmaven.repo.local=/tmp/m2-pinot-9989 -pl pinot-segment-spi,pinot-segment-local -am -Dtest=ColumnMetadataImplTest,DefaultColumnHandlerTest -Dsurefire.failIfNoSpecifiedTests=false test`
- `./mvnw -Dmaven.repo.local=/tmp/m2-pinot-9989 -pl pinot-segment-spi,pinot-segment-local spotless:apply checkstyle:check license:check`
- `git diff --check`
The tests cover metadata round-trip, missing metadata backward compatibility, transform-function action-map decisions, regeneration when a legacy auto-generated derived column lacks stored transform metadata, and persistence of transform metadata when Pinot falls back to default values because a transform argument is absent.
---
Drafted-by: Codex (GPT-5) (no human review before posting)
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Regenerate derived columns when transforms change [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on code in PR #18977:
URL: https://github.com/apache/pinot/pull/18977#discussion_r3725704953
##########
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:
Good catch, thanks. The method does return null when there is no ingestion transform config and no schema-level transform. I will add @Nullable (javax.annotation.Nullable, already imported in this file) to the return type of getTransformFunctionForColumn to make the contract explicit, matching how createDefaultValueColumnV1Indices annotates its transformFunction parameter.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Regenerate derived columns when transforms change [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Regenerate derived columns when transforms change [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #18977:
URL: https://github.com/apache/pinot/pull/18977#issuecomment-5642841034
@J-HowHuang Yes, once the transform is added, updated, or removed, reload applies that to existing auto-generated derived columns. Legacy segments that have no stored transform are a metadata-only backfill on first reload, then the next real change rebuilds values.
There is no opt-out in this PR for keeping the old transform on old segments. If we want that door, I would rather do it as a follow-up than add a public flag here.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Regenerate derived columns when transforms change [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on code in PR #18977:
URL: https://github.com/apache/pinot/pull/18977#discussion_r3724861019
##########
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:
For legacy segments with derived columns, they'll come with `null` transform function from metadata and guaranteed a segment rebuild? Is it better to have a no-op for this 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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Regenerate derived columns when transforms change [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reload derived-column transforms without freezing or stale star-trees [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on code in PR #18977:
URL: https://github.com/apache/pinot/pull/18977#discussion_r4051878950
##########
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:
I took another look at this while adding versioned provenance. I kept the legacy overload because callers that cannot prove how the values were produced need to leave provenance unknown. It delegates to the new overload and clears the provenance keys. Transform-aware generation paths use the new overload directly. This avoids labeling source or mixed values as a known no-transform state.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reload derived-column transforms without freezing or stale star-trees [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #18977:
URL: https://github.com/apache/pinot/pull/18977#issuecomment-5658363744
@Jackie-Jiang @J-HowHuang Ready for another look on HEAD `d858652`.
Since the Aug 13 notes: BACKFILL writes only `transformFunctionBackfilled`, expressions stay on `getProperty()`, minion RefreshSegment skips transform BACKFILL/UPDATE so it cannot freeze the column, star-trees rebuild on VALUES_CHANGED, and persist fails closed on surrogates. Apache CI is green on this head.
J-How's earlier +1 was on `16505955` and does not cover this head. Formal reviewer requests 403 from this account, so tagging here.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reload derived-column transforms without freezing or stale star-trees [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on code in PR #18977:
URL: https://github.com/apache/pinot/pull/18977#discussion_r4051880772
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessor.java:
##########
@@ -409,15 +431,21 @@ private boolean processStarTrees(File indexDir,
boolean shouldGenerateStarTree = !starTreeBuilderConfigs.isEmpty();
boolean shouldRemoveStarTree = false;
+ boolean transformValuesChangedOnStarTreeColumns = false;
List<StarTreeV2Metadata> starTreeMetadataList = segmentMetadata.getStarTreeV2MetadataList();
if (starTreeMetadataList != null) {
// There are existing star-trees
+ transformValuesChangedOnStarTreeColumns =
+ StarTreeBuilderUtils.usesAnyColumn(starTreeMetadataList, _columnsWithChangedTransformValues);
Review Comment:
Fixed. When dynamic creation is disabled, a value change on a star-tree column now removes the stale tree instead of leaving it queryable. The regression test verifies the updated column values and confirms that no stale star-tree remains.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessor.java:
##########
@@ -437,9 +465,11 @@ private boolean processStarTrees(File indexDir,
StarTreeBuilderUtils.removeStarTrees(indexDir);
} else {
// NOTE: Always use OFF_HEAP mode on server side.
- // Pass _indexLoadingConfig so downstream readers can resolve table-level configs we set
+ // Pass _indexLoadingConfig so downstream readers can resolve table-level configs we set.
+ // Force rebuild when transform values changed: reuse would keep stale aggregates because star-tree
+ // config is unchanged after UPDATE_*_TRANSFORM_FUNCTION.
MultipleTreesBuilder builder = new MultipleTreesBuilder(starTreeBuilderConfigs, indexDir,
- MultipleTreesBuilder.BuildMode.OFF_HEAP, _indexLoadingConfig);
+ MultipleTreesBuilder.BuildMode.OFF_HEAP, _indexLoadingConfig, transformValuesChangedOnStarTreeColumns);
Review Comment:
Fixed. A forced rebuild after a transform value change no longer restores the previous tree if building or finalization fails. The stale backup and metadata are removed, so queries fall back to the regenerated forward indexes. Ordinary rebuild failures still restore the previous tree. Tests cover both cases.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:
##########
@@ -298,20 +354,39 @@ Map<String, DefaultColumnAction> computeDefaultColumnActionMap() {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_METRIC_DEFAULT_VALUE);
} else if (isSingleValueInMetadata != isSingleValueInSchema) {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_METRIC_NUMBER_OF_VALUES);
+ } else if (isTransformFunctionChanged(column, columnMetadata)) {
+ defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_METRIC_TRANSFORM_FUNCTION);
}
} else if (fieldTypeInMetadata == DATE_TIME) {
if (dataTypeInMetadata != dataTypeInSchema) {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_DATE_TIME_DATA_TYPE);
} else if (!defaultValueInSchema.equals(defaultValueInMetadata)) {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_DATE_TIME_DEFAULT_VALUE);
+ } else if (isTransformFunctionChanged(column, columnMetadata)) {
+ defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_DATE_TIME_TRANSFORM_FUNCTION);
}
} else if (fieldTypeInMetadata == COMPLEX) {
if (dataTypeInMetadata != dataTypeInSchema) {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_COMPLEX_DATA_TYPE);
} else if (!defaultValueInSchema.equals(defaultValueInMetadata)) {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_COMPLEX_DEFAULT_VALUE);
+ } else if (isTransformFunctionChanged(column, columnMetadata)) {
+ defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_COMPLEX_TRANSFORM_FUNCTION);
}
}
+
+ // Segments created before the transform function was tracked in the metadata report null for both the stored
+ // and the backfilled field. Their values cannot be told apart from up-to-date ones, so instead of regenerating
+ // them, record the configured transform in TRANSFORM_FUNCTION_BACKFILLED (values untouched) so that the NEXT
+ // transform function change is detected. The expression is not written to TRANSFORM_FUNCTION, which is reserved
+ // for transforms that actually produced the stored values.
+ // Tradeoff: a transform function change that lands in the very same reload as this backfill is not applied to
+ // the existing values (which matches the behavior before the transform function was tracked at all); operators
+ // who need those values regenerated can force it with one more change to the expression.
+ if (!defaultColumnActionMap.containsKey(column) && getEffectiveTransformFunction(columnMetadata) == null
+ && getTransformFunctionForColumn(column) != null) {
+ defaultColumnActionMap.put(column, DefaultColumnAction.BACKFILL_TRANSFORM_FUNCTION);
Review Comment:
Fixed with the provenance version marker. A removed transform is recorded as a known no-transform state instead of legacy unknown metadata. Restoring the original expression therefore triggers regeneration instead of metadata-only handling. The regression test covers the complete A to none to A sequence.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:
##########
@@ -138,6 +160,7 @@ boolean isRemoveAction() {
protected final TableConfig _tableConfig;
protected final Schema _schema;
protected final SegmentDirectory.Writer _segmentWriter;
+ private final Map<String, String> _transformFunctionByColumn;
Review Comment:
Updated. `DefaultColumnAction` remains protected in `BaseDefaultColumnHandler`.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -283,6 +283,15 @@ public static ImmutableSegment load(SegmentDirectory segmentDirectory, IndexLoad
/// segment format, adding new indices or updating default columns.
public static boolean needPreprocess(SegmentDirectory segmentDirectory, IndexLoadingConfig indexLoadingConfig)
throws Exception {
+ return needPreprocess(segmentDirectory, indexLoadingConfig, true);
+ }
+
+ /// Same as [#needPreprocess(SegmentDirectory, IndexLoadingConfig)], with an option to ignore transform-function
+ /// BACKFILL/UPDATE. Record-replay rebuild (RefreshSegment) must not treat those as a rebuild signal; server
+ /// [SegmentPreProcessor] + DefaultColumnHandler is the apply path.
+ public static boolean needPreprocess(SegmentDirectory segmentDirectory, IndexLoadingConfig indexLoadingConfig,
Review Comment:
Agreed. The BACKFILL action is gone. Legacy segments without provenance remain untouched, and only columns with known provenance receive transform-update actions. RefreshSegment now recomputes changed transforms during record replay, including dependency chains, instead of suppressing the update and preserving stale values. Tests cover legacy segments, direct changes, remove and restore, non-auto-generated outputs, and chained transforms.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reload derived-column transforms without freezing or stale star-trees [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18977:
URL: https://github.com/apache/pinot/pull/18977#discussion_r4020596966
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -283,6 +283,15 @@ public static ImmutableSegment load(SegmentDirectory segmentDirectory, IndexLoad
/// segment format, adding new indices or updating default columns.
public static boolean needPreprocess(SegmentDirectory segmentDirectory, IndexLoadingConfig indexLoadingConfig)
throws Exception {
+ return needPreprocess(segmentDirectory, indexLoadingConfig, true);
+ }
+
+ /// Same as [#needPreprocess(SegmentDirectory, IndexLoadingConfig)], with an option to ignore transform-function
+ /// BACKFILL/UPDATE. Record-replay rebuild (RefreshSegment) must not treat those as a rebuild signal; server
+ /// [SegmentPreProcessor] + DefaultColumnHandler is the apply path.
+ public static boolean needPreprocess(SegmentDirectory segmentDirectory, IndexLoadingConfig indexLoadingConfig,
Review Comment:
The only place where `includeTransformFunctionActions` is passed as `false` is from `RefreshSegemntTaskExecutor`, where the usage itself is wrong (there is a TODO for it).
I do see the problem of backfilling always triggering segment refresh, bug update should trigger refresh.
Based on this, I'd suggest removing backfill and only support updates on new generated segment where transform function is persisted. This way the entire flow can be simplified a lot.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:
##########
@@ -138,6 +160,7 @@ boolean isRemoveAction() {
protected final TableConfig _tableConfig;
protected final Schema _schema;
protected final SegmentDirectory.Writer _segmentWriter;
+ private final Map<String, String> _transformFunctionByColumn;
Review Comment:
Keep it `protected` in base class
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reload derived-column transforms without freezing or stale star-trees [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on code in PR #18977:
URL: https://github.com/apache/pinot/pull/18977#discussion_r4051880393
##########
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. Segment creation now builds the column-to-transform map once through `IngestionConfigUtils.getTransformFunctionByColumn`. The ingestion config takes precedence, with schema transforms as the fallback. The default-column path uses the same helper.
##########
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:
I kept compatibility defaults here because implementations that do not know about provenance need to retain the legacy behavior. The defaults return unknown provenance. `ColumnMetadataImpl`, `EmptyColumnMetadata`, and `SimpleColumnMetadata` provide the concrete values. This prevents older internal implementations from being treated as known no-transform columns.
##########
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 this. Spotless and Checkstyle are clean on the affected modules.
##########
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 kept `getProperty()` for the legacy raw key because `getString()` performs Commons Configuration interpolation and can rewrite `${...}` inside a Groovy expression. New metadata is stored as Base64-encoded UTF-8 and decoded directly. Tests cover Unicode, `${...}`, commas, quotes, and backslashes.
##########
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:
I reworked this into versioned provenance instead of adding another backfill field. Legacy columns have no provenance version and remain untouched. Known transforms and known no-transform states carry a version, so removing and later restoring a transform rebuilds the values both times. Tests cover the legacy no-op and the complete remove-and-restore sequence.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/BaseSegmentCreator.java:
##########
@@ -694,6 +700,52 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
}
}
+ /// Records a transform that actually produced the stored column values.
+ public static void addTransformFunction(PropertiesConfiguration properties, String column,
+ @Nullable String transformFunction) {
+ addTransformFunction(properties, column, transformFunction, false);
+ }
+
+ /// Records transform provenance for a column.
+ /// A real stored transform is written to [V1Constants.MetadataKeys.Column#TRANSFORM_FUNCTION]. A backward-compat
+ /// backfill (values were not regenerated from the expression) is written only to
+ /// [V1Constants.MetadataKeys.Column#TRANSFORM_FUNCTION_BACKFILLED], so the two cases stay distinguishable.
+ public static void addTransformFunction(PropertiesConfiguration properties, String column,
+ @Nullable String transformFunction, boolean backfilled) {
+ String transformFunctionKey = getKeyFor(column, TRANSFORM_FUNCTION);
+ String backfilledKey = getKeyFor(column, TRANSFORM_FUNCTION_BACKFILLED);
+ if (transformFunction == null) {
+ properties.clearProperty(transformFunctionKey);
+ properties.clearProperty(backfilledKey);
+ return;
+ }
+ String validTransformFunction = CommonsConfigurationUtils.replaceSpecialCharacterInPropertyValue(transformFunction);
+ if (validTransformFunction == null) {
+ throw new IllegalArgumentException("Cannot persist transform function for column: " + column
+ + " because it contains UTF-16 surrogate characters that segment metadata cannot store");
Review Comment:
Fixed by storing new transform metadata as Base64-encoded UTF-8. The raw property remains only as a compatibility reader for earlier branch artifacts. Tests cover emoji, `${...}`, commas, quotes, and backslashes.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessor.java:
##########
@@ -159,6 +161,8 @@ public void process(@Nullable SegmentOperationsThrottlerSet segmentOperationsThr
DefaultColumnHandler defaultColumnHandler =
DefaultColumnHandlerFactory.getDefaultColumnHandler(indexDir, segmentMetadata, _indexLoadingConfig,
segmentWriter);
+ // Capture before apply: UPDATE_*_TRANSFORM_FUNCTION changes values; BACKFILL does not.
+ _columnsWithChangedTransformValues = defaultColumnHandler.getColumnsWithPendingTransformValueChanges();
Review Comment:
Fixed. A transform value change now rebuilds the shared multi-column text index when the changed column participates in it. The regression test verifies that the old term disappears and the new term becomes searchable.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Regenerate derived columns when transforms change [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #18977:
URL: https://github.com/apache/pinot/pull/18977#issuecomment-5186528557
@J-HowHuang Could you please help review this PR?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Regenerate derived columns when transforms change [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Regenerate derived columns when transforms change [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Regenerate derived columns when transforms change [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18977:
URL: https://github.com/apache/pinot/pull/18977#issuecomment-5200411619
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18977?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `38.88889%` with `55 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.11%. Comparing base ([`cd2538d`](https://app.codecov.io/gh/apache/pinot/commit/cd2538d4b7bd0d54c722173db66c0ccaed646ee2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`6cbca04`](https://app.codecov.io/gh/apache/pinot/commit/6cbca04420f14ace5105b6c7940dbf77d49d815b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18977?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...loader/defaultcolumn/BaseDefaultColumnHandler.java](https://app.codecov.io/gh/apache/pinot/pull/18977?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Findex%2Floader%2Fdefaultcolumn%2FBaseDefaultColumnHandler.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2xvYWRlci9kZWZhdWx0Y29sdW1uL0Jhc2VEZWZhdWx0Q29sdW1uSGFuZGxlci5qYXZh) | 16.36% | [43 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18977?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...local/segment/creator/impl/BaseSegmentCreator.java](https://app.codecov.io/gh/apache/pinot/pull/18977?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Fcreator%2Fimpl%2FBaseSegmentCreator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9CYXNlU2VnbWVudENyZWF0b3IuamF2YQ==) | 75.00% | [2 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18977?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../local/segment/index/map/SimpleColumnMetadata.java](https://app.codecov.io/gh/apache/pinot/pull/18977?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Findex%2Fmap%2FSimpleColumnMetadata.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L21hcC9TaW1wbGVDb2x1bW5NZXRhZGF0YS5qYXZh) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18977?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...a/org/apache/pinot/segment/spi/ColumnMetadata.java](https://app.codecov.io/gh/apache/pinot/pull/18977?src=pr&el=tree&filepath=pinot-segment-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Fspi%2FColumnMetadata.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL0NvbHVtbk1ldGFkYXRhLmphdmE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18977?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...segment/spi/index/metadata/ColumnMetadataImpl.java](https://app.codecov.io/gh/apache/pinot/pull/18977?src=pr&el=tree&filepath=pinot-segment-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Fspi%2Findex%2Fmetadata%2FColumnMetadataImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L21ldGFkYXRhL0NvbHVtbk1ldGFkYXRhSW1wbC5qYXZh) | 91.66% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18977?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...egment/spi/index/metadata/EmptyColumnMetadata.java](https://app.codecov.io/gh/apache/pinot/pull/18977?src=pr&el=tree&filepath=pinot-segment-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Fspi%2Findex%2Fmetadata%2FEmptyColumnMetadata.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L21ldGFkYXRhL0VtcHR5Q29sdW1uTWV0YWRhdGEuamF2YQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18977?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (cd2538d) and HEAD (6cbca04). Click for more details.
>
> <details><summary>HEAD has 16 uploads less than BASE</summary>
>
>| Flag | BASE (cd2538d) | HEAD (6cbca04) |
>|------|------|------|
>|unittests|2|1|
>|java-25|5|1|
>|temurin|5|1|
>|unittests2|1|0|
>|integration|3|0|
>|integration1|1|0|
>|custom-integration1|1|0|
>|integration2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18977 +/- ##
============================================
- Coverage 66.59% 57.11% -9.48%
+ Complexity 1423 1 -1422
============================================
Files 3443 2646 -797
Lines 218536 158932 -59604
Branches 34780 26126 -8654
============================================
- Hits 145538 90778 -54760
+ Misses 61273 60360 -913
+ Partials 11725 7794 -3931
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18977/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18977/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18977/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/18977/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/18977/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/18977/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.11% <38.88%> (-9.48%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18977/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.11% <38.88%> (-9.48%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18977/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.11% <38.88%> (-9.48%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18977/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.11% <38.88%> (-0.01%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18977/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/18977?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Regenerate derived columns when transforms change [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #18977:
URL: https://github.com/apache/pinot/pull/18977#issuecomment-4949200049
Review request: @Jackie-Jiang
GitHub did not allow this account/integration to add requested reviewers through the reviewer section, so I am tagging the relevant reviewer here for visibility.
This PR fixes #9989 by persisting derived-column transform-function metadata into segment column metadata and using that metadata during reload to regenerate auto-generated derived columns when the configured transform changes. Review would be especially useful because the implementation follows the issue guidance to persist the transform function and compare it against the table config during reload.
---
Drafted-by: Codex (GPT-5); human-reviewed by Vamsi-klu before posting
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reload derived-column transforms without freezing or stale star-trees [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18977:
URL: https://github.com/apache/pinot/pull/18977#discussion_r4020495767
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/BaseSegmentCreator.java:
##########
@@ -694,6 +700,52 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
}
}
+ /// Records a transform that actually produced the stored column values.
+ public static void addTransformFunction(PropertiesConfiguration properties, String column,
+ @Nullable String transformFunction) {
+ addTransformFunction(properties, column, transformFunction, false);
+ }
+
+ /// Records transform provenance for a column.
+ /// A real stored transform is written to [V1Constants.MetadataKeys.Column#TRANSFORM_FUNCTION]. A backward-compat
+ /// backfill (values were not regenerated from the expression) is written only to
+ /// [V1Constants.MetadataKeys.Column#TRANSFORM_FUNCTION_BACKFILLED], so the two cases stay distinguishable.
+ public static void addTransformFunction(PropertiesConfiguration properties, String column,
+ @Nullable String transformFunction, boolean backfilled) {
+ String transformFunctionKey = getKeyFor(column, TRANSFORM_FUNCTION);
+ String backfilledKey = getKeyFor(column, TRANSFORM_FUNCTION_BACKFILLED);
+ if (transformFunction == null) {
+ properties.clearProperty(transformFunctionKey);
+ properties.clearProperty(backfilledKey);
+ return;
+ }
+ String validTransformFunction = CommonsConfigurationUtils.replaceSpecialCharacterInPropertyValue(transformFunction);
+ if (validTransformFunction == null) {
+ throw new IllegalArgumentException("Cannot persist transform function for column: " + column
+ + " because it contains UTF-16 surrogate characters that segment metadata cannot store");
Review Comment:
**Preserve valid Unicode transforms when storing provenance (new regression).**
`replaceSpecialCharacterInPropertyValue()` rejects every UTF-16 surrogate, including the valid pair representing an emoji. An existing expression such as `concat(source, '😀')` now fails normal segment generation/sealing because `addColumnMetadataInfo()` calls this writer for ordinary ingested columns too. The failure is not limited to legacy BACKFILL.
The expression can be represented reversibly; the current helper refuses to encode it. Consider a metadata encoding supporting full Unicode, with matching decoding and an encoding marker or separate key, so provenance is retained without rejecting previously valid transforms. A normal segment-generation round-trip test would cover the compatibility case beyond the current tests that expect BACKFILL to throw.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessor.java:
##########
@@ -159,6 +161,8 @@ public void process(@Nullable SegmentOperationsThrottlerSet segmentOperationsThr
DefaultColumnHandler defaultColumnHandler =
DefaultColumnHandlerFactory.getDefaultColumnHandler(indexDir, segmentMetadata, _indexLoadingConfig,
segmentWriter);
+ // Capture before apply: UPDATE_*_TRANSFORM_FUNCTION changes values; BACKFILL does not.
+ _columnsWithChangedTransformValues = defaultColumnHandler.getColumnsWithPendingTransformValueChanges();
Review Comment:
**Also invalidate multi-column text indexes after transform value changes (existing gap, new trigger).**
If a rewritten derived STRING column participates in a multi-column text index, `removeColumnIndices()` removes its individual indexes but leaves the shared `multi_col_text_idx` files and metadata. Later, `processMultiColTextIndex()` only compares index configuration and skips rebuilding when that configuration is unchanged. `TEXT_MATCH` therefore continues searching the old values while the forward index contains the new values.
This gap already exists for schema/default-value updates; this PR makes it reachable through transform-only updates as well. Consider using the changed-column set to invalidate/rebuild the shared text index when it intersects the indexed columns, with a regression test that queries the updated text.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessor.java:
##########
@@ -437,9 +465,11 @@ private boolean processStarTrees(File indexDir,
StarTreeBuilderUtils.removeStarTrees(indexDir);
} else {
// NOTE: Always use OFF_HEAP mode on server side.
- // Pass _indexLoadingConfig so downstream readers can resolve table-level configs we set
+ // Pass _indexLoadingConfig so downstream readers can resolve table-level configs we set.
+ // Force rebuild when transform values changed: reuse would keep stale aggregates because star-tree
+ // config is unchanged after UPDATE_*_TRANSFORM_FUNCTION.
MultipleTreesBuilder builder = new MultipleTreesBuilder(starTreeBuilderConfigs, indexDir,
- MultipleTreesBuilder.BuildMode.OFF_HEAP, _indexLoadingConfig);
+ MultipleTreesBuilder.BuildMode.OFF_HEAP, _indexLoadingConfig, transformValuesChangedOnStarTreeColumns);
Review Comment:
**Avoid restoring an old tree after a value-changing rebuild fails (existing fallback, new failure scenario).**
The transformed values and new expression metadata are saved before this forced rebuild. If `singleTreeBuilder.build()` fails, such as on an I/O error during generation, `MultipleTreesBuilder.close()` restores the previous tree and its metadata, while the catch below suppresses the build exception. Preprocessing can then succeed with new column values and old aggregates. The expression and tree configuration now match, so a subsequent reload also has no signal to retry.
Restoring old trees is existing behavior, but the forced rebuild introduces this use after transform values have changed. Consider invalidating/removing the stale tree on this path or rolling back the complete segment state. An injected build-failure test after the column rewrite would cover it.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:
##########
@@ -298,20 +354,39 @@ Map<String, DefaultColumnAction> computeDefaultColumnActionMap() {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_METRIC_DEFAULT_VALUE);
} else if (isSingleValueInMetadata != isSingleValueInSchema) {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_METRIC_NUMBER_OF_VALUES);
+ } else if (isTransformFunctionChanged(column, columnMetadata)) {
+ defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_METRIC_TRANSFORM_FUNCTION);
}
} else if (fieldTypeInMetadata == DATE_TIME) {
if (dataTypeInMetadata != dataTypeInSchema) {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_DATE_TIME_DATA_TYPE);
} else if (!defaultValueInSchema.equals(defaultValueInMetadata)) {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_DATE_TIME_DEFAULT_VALUE);
+ } else if (isTransformFunctionChanged(column, columnMetadata)) {
+ defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_DATE_TIME_TRANSFORM_FUNCTION);
}
} else if (fieldTypeInMetadata == COMPLEX) {
if (dataTypeInMetadata != dataTypeInSchema) {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_COMPLEX_DATA_TYPE);
} else if (!defaultValueInSchema.equals(defaultValueInMetadata)) {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_COMPLEX_DEFAULT_VALUE);
+ } else if (isTransformFunctionChanged(column, columnMetadata)) {
+ defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_COMPLEX_TRANSFORM_FUNCTION);
}
}
+
+ // Segments created before the transform function was tracked in the metadata report null for both the stored
+ // and the backfilled field. Their values cannot be told apart from up-to-date ones, so instead of regenerating
+ // them, record the configured transform in TRANSFORM_FUNCTION_BACKFILLED (values untouched) so that the NEXT
+ // transform function change is detected. The expression is not written to TRANSFORM_FUNCTION, which is reserved
+ // for transforms that actually produced the stored values.
+ // Tradeoff: a transform function change that lands in the very same reload as this backfill is not applied to
+ // the existing values (which matches the behavior before the transform function was tracked at all); operators
+ // who need those values regenerated can force it with one more change to the expression.
+ if (!defaultColumnActionMap.containsKey(column) && getEffectiveTransformFunction(columnMetadata) == null
+ && getTransformFunctionForColumn(column) != null) {
+ defaultColumnActionMap.put(column, DefaultColumnAction.BACKFILL_TRANSFORM_FUNCTION);
Review Comment:
**Distinguish known absence of a transform from legacy missing provenance (new remove/restore regression).**
Starting with an auto-generated column produced by transform A: remove A and reload, then restore A and reload again. The first reload regenerates default values and clears both provenance fields. The second reload reaches this legacy BACKFILL branch, which records A without regenerating values. Further reloads are no-ops, leaving the defaults permanently despite restoring the original config.
Before this PR, that remove/restore sequence left the original values untouched. Consider persisting a distinct known-no-transform state or metadata version so this transition regenerates values while genuinely legacy segments still receive metadata-only backfill. Extending `testRemovingTransformFunctionFromConfigRegeneratesDefaultValues` to restore the expression would cover the sequence.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessor.java:
##########
@@ -409,15 +431,21 @@ private boolean processStarTrees(File indexDir,
boolean shouldGenerateStarTree = !starTreeBuilderConfigs.isEmpty();
boolean shouldRemoveStarTree = false;
+ boolean transformValuesChangedOnStarTreeColumns = false;
List<StarTreeV2Metadata> starTreeMetadataList = segmentMetadata.getStarTreeV2MetadataList();
if (starTreeMetadataList != null) {
// There are existing star-trees
+ transformValuesChangedOnStarTreeColumns =
+ StarTreeBuilderUtils.usesAnyColumn(starTreeMetadataList, _columnsWithChangedTransformValues);
Review Comment:
**Invalidate affected star-trees even when dynamic creation is disabled (existing gap, new trigger).**
`processStarTrees()` returns at the `isEnableDynamicStarTreeCreation()` guard before reaching this check. With an existing tree over an auto-generated column—for example, one built while dynamic creation was enabled and retained after disabling it—the new transform-update path still rewrites the column first. The old tree remains queryable against the new column values.
The early return and similar structural-update gap predate this PR. For the new transform path, invalidation needs to be independent of permission to create trees: remove affected trees, rebuild them, or prevent committing inconsistent state. The reload test could disable dynamic creation after the initial tree is built and verify that stale aggregates are never served.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reload derived-column transforms without freezing or stale star-trees [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18977:
URL: https://github.com/apache/pinot/pull/18977#discussion_r4109596149
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessor.java:
##########
@@ -160,6 +163,9 @@ public void process(@Nullable SegmentOperationsThrottlerSet segmentOperationsThr
DefaultColumnHandlerFactory.getDefaultColumnHandler(indexDir, segmentMetadata, _indexLoadingConfig,
segmentWriter);
defaultColumnHandler.updateDefaultColumns();
+ // Invalidate dependent indexes only for values that were actually regenerated. A tolerated column-build failure
+ // leaves the original values and their indexes valid.
+ _columnsWithChangedTransformValues = defaultColumnHandler.getColumnsWithChangedTransformValues();
Review Comment:
[CRITICAL] This changed-column signal survives only in memory. `updateDefaultColumns()` saves replacement values and provenance before StarTree and multi-column text indexes are rebuilt in the later preprocessing phase. If the process stops between those phases, the next load sees current provenance and matching index configs, so it can retain indexes over the old values and return stale results. Please durably invalidate affected indexes before committing the column replacement, or persist a pending-rebuild marker until all dependent indexes have been rebuilt.
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/refreshsegment/RefreshSegmentTaskExecutor.java:
##########
@@ -196,6 +371,60 @@ private static SegmentGeneratorConfig getSegmentGeneratorConfig(File workingDir,
return config;
}
+ private static void preserveOriginalColumnMetadata(File refreshedSegmentDir,
+ Map<String, ColumnMetadata> originalMetadata, Set<String> transformColumnsToRecompute,
+ Set<String> changedTransformColumns, Map<String, String> transformFunctionByColumn,
+ Set<String> replayDefaultColumns)
+ throws Exception {
+ if (originalMetadata.isEmpty() && replayDefaultColumns.isEmpty()) {
+ return;
+ }
+ var properties = SegmentMetadataUtils.getPropertiesConfiguration(refreshedSegmentDir);
+ for (Map.Entry<String, ColumnMetadata> entry : originalMetadata.entrySet()) {
+ String column = entry.getKey();
+ ColumnMetadata columnMetadata = entry.getValue();
+ properties.setProperty(V1Constants.MetadataKeys.Column.getKeyFor(column,
Review Comment:
[CRITICAL] Restoring the old `isAutoGenerated=false` flag is incorrect when a known ingestion transform has been removed. The first refresh omits its output and fills it with the schema default, but this line then marks the rebuilt value as a source value. If the default later changes (for example 0 to 5), subsequent refreshes keep the stored 0 and the server default-column handler skips the column. Please mark an output regenerated from a schema default as auto-generated.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/TransformPipeline.java:
##########
@@ -52,23 +55,43 @@ public TransformPipeline(String tableNameWithType, List<RecordTransformer> trans
_tableNameWithType = tableNameWithType;
_transformers = transformers;
FilterTransformer filterTransformer = null;
+ ExpressionTransformer expressionTransformer = null;
Set<String> cumulativeInputColumns = new HashSet<>();
for (int i = transformers.size() - 1; i >= 0; i--) {
RecordTransformer recordTransformer = transformers.get(i);
if (recordTransformer instanceof FilterTransformer) {
filterTransformer = (FilterTransformer) recordTransformer;
}
+ if (recordTransformer instanceof ExpressionTransformer) {
+ expressionTransformer = (ExpressionTransformer) recordTransformer;
+ }
recordTransformer.withInputColumnsForDownstreamTransformers(cumulativeInputColumns);
cumulativeInputColumns.addAll(recordTransformer.getInputColumns());
}
_inputColumns = cumulativeInputColumns;
_filterTransformer = filterTransformer;
+ _expressionTransformer = expressionTransformer;
}
public TransformPipeline(TableConfig tableConfig, Schema schema) {
this(tableConfig.getTableName(), RecordTransformerUtils.getDefaultTransformers(tableConfig, schema));
}
+ /// Creates a pipeline for replaying stored rows while preserving authoritative values for selected transform output
+ /// columns. Presence in the decoded row, including a null marker, suppresses expression re-evaluation.
+ public TransformPipeline(TableConfig tableConfig, Schema schema, Set<String> columnsToPreserve) {
Review Comment:
[MINOR] Neither new replay constructor is called on this PR head. RefreshSegment builds its transformer list and calls the existing `(String, List<RecordTransformer>)` constructor. The three-argument `RecordTransformerUtils.getDefaultTransformers` overload exists only to support these unused constructors. Please remove the unused overloads to keep the API surface small.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/ColumnMetadataTest.java:
##########
@@ -316,7 +316,7 @@ public void testMaxRowLengthInBytesPersistedForMvVarLength() {
PropertiesConfiguration config = new PropertiesConfiguration();
BaseSegmentCreator.addColumnMetadataInfo(config, "mvString", stats, 10, fieldSpec, false, -1,
- FieldConfig.EncodingType.RAW, false);
+ FieldConfig.EncodingType.RAW, false, null);
Review Comment:
[MINOR] This and the two similar changes below switch unrelated metadata tests to the overload that stamps known-no-transform provenance. The tests assert row-length and field-spec behavior, not provenance, and the original overload still exists. Please revert these fixture changes so they exercise only the behavior under test.
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/refreshsegment/RefreshSegmentTaskExecutor.java:
##########
@@ -145,12 +189,24 @@ protected SegmentConversionResult convert(PinotTaskConfig pinotTaskConfig, File
// honored (needPreprocess=false: read-only).
ImmutableSegment segment = ImmutableSegmentLoader.load(indexDir, indexLoadingConfig, false);
try (PinotSegmentRecordReader recordReader = new PinotSegmentRecordReader()) {
- recordReader.init(segment);
- SegmentGeneratorConfig config = getSegmentGeneratorConfig(workingDir, tableConfig, segmentMetadata, segmentName,
- getSchema(tableNameWithType));
+ Set<String> fieldsToRead = new HashSet<>(segment.getPhysicalColumnNames());
+ fieldsToRead.removeAll(transformColumnsToRecompute);
+ recordReader.initWithFieldsToRead(segment, fieldsToRead);
+ SegmentGeneratorConfig config =
+ getSegmentGeneratorConfig(workingDir, tableConfig, segmentMetadata, segmentName, schema);
SegmentIndexCreationDriverImpl driver = new SegmentIndexCreationDriverImpl();
- driver.init(config, recordReader);
+ List<RecordTransformer> recordTransformers = new ArrayList<>();
+ if (!replayDefaultValues.isEmpty()) {
Review Comment:
[CRITICAL] `ReplayDefaultValueTransformer` runs before the configured enrichers. For a new schema column populated by an enricher, it first calls `putDefaultNullValue`, setting a null marker; the enricher later writes the real value with `putValue`, which does not clear that marker. Segment creation writes the column into the null vector, so null-enabled queries see NULL despite the enriched value. Please inject defaults after enrichment and before expression evaluation, only for fields that remain absent, or clear the marker when a producer supplies a value.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/openstruct/OpenStructColumnSplitter.java:
##########
@@ -442,7 +442,7 @@ private void writeDenseKeyColumn(String key)
FieldConfig.EncodingType encoding =
useDictionary ? FieldConfig.EncodingType.DICTIONARY : FieldConfig.EncodingType.RAW;
BaseSegmentCreator.addColumnMetadataInfo(props, materializedCol, statsCollector, _numDocs, childFieldSpec,
- useDictionary, dictElementSize, encoding, false);
+ useDictionary, dictElementSize, encoding, false, null);
Review Comment:
[MINOR] Passing `null` to the new overload stamps materialized OpenStruct children as known-no-transform rather than leaving their provenance unknown. The same change appears for sparse children below. Is this metadata behavior needed for the derived-column fix? If not, please keep the original overload and avoid changing OpenStruct child metadata in this PR.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:
##########
@@ -298,23 +445,33 @@ Map<String, DefaultColumnAction> computeDefaultColumnActionMap() {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_METRIC_DEFAULT_VALUE);
} else if (isSingleValueInMetadata != isSingleValueInSchema) {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_METRIC_NUMBER_OF_VALUES);
+ } else if (isTransformFunctionChanged(column, columnMetadata)) {
+ defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_METRIC_TRANSFORM_FUNCTION);
}
} else if (fieldTypeInMetadata == DATE_TIME) {
if (dataTypeInMetadata != dataTypeInSchema) {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_DATE_TIME_DATA_TYPE);
} else if (!defaultValueInSchema.equals(defaultValueInMetadata)) {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_DATE_TIME_DEFAULT_VALUE);
+ } else if (isTransformFunctionChanged(column, columnMetadata)) {
+ defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_DATE_TIME_TRANSFORM_FUNCTION);
}
} else if (fieldTypeInMetadata == COMPLEX) {
if (dataTypeInMetadata != dataTypeInSchema) {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_COMPLEX_DATA_TYPE);
} else if (!defaultValueInSchema.equals(defaultValueInMetadata)) {
defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_COMPLEX_DEFAULT_VALUE);
+ } else if (isTransformFunctionChanged(column, columnMetadata)) {
+ defaultColumnActionMap.put(column, DefaultColumnAction.UPDATE_COMPLEX_TRANSFORM_FUNCTION);
}
}
} else {
// Column does not exist in the segment, add default value for it.
+ if (_columnsInTransformChains.contains(column)) {
Review Comment:
[CRITICAL] This skips ADD for a newly introduced transform output when it depends on an existing auto-generated column. For example, if the segment has default column `A` and the schema adds `B = plus(A, 1)`, normal server reload leaves `B` absent and queries serve its virtual schema default instead of `A + 1`. RefreshSegment is optional and is not scheduled for tables without task configuration. Please materialize this chain before the reloaded segment is served, either in place when its inputs are stable or through required record replay.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/map/SimpleColumnMetadata.java:
##########
@@ -132,6 +132,17 @@ public boolean isAutoGenerated() {
return false;
}
+ @Nullable
Review Comment:
[MINOR] These two overrides return exactly the defaults already supplied by `ColumnMetadata`. The same duplicate overrides were added to `EmptyColumnMetadata`. Please remove both pairs and inherit the interface defaults.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org