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