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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
