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: [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]
