Jackie-Jiang commented on code in PR #18977:
URL: https://github.com/apache/pinot/pull/18977#discussion_r4499108744
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/refreshsegment/RefreshSegmentTaskExecutor.java:
##########
@@ -196,6 +374,74 @@ 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();
+ // A column whose transform was removed was just regenerated from the
schema default, which makes it a default
+ // column even if the transform ran at ingestion time: a later
default-value change must regenerate it.
+ boolean regeneratedFromDefault =
+ changedTransformColumns.contains(column) &&
transformFunctionByColumn.get(column) == null;
+ properties.setProperty(V1Constants.MetadataKeys.Column.getKeyFor(column,
+ V1Constants.MetadataKeys.Column.IS_AUTO_GENERATED),
+ regeneratedFromDefault || columnMetadata.isAutoGenerated());
+ if (changedTransformColumns.contains(column)) {
+ // A removed transform is deliberately regenerated as schema default
values. Record that known no-transform
+ // state so adding an expression later is distinguishable from legacy
metadata with unknown provenance.
+ if (transformFunctionByColumn.get(column) == null) {
+ BaseSegmentCreator.addTransformFunction(properties, column, null);
+ }
+ } else if (!transformColumnsToRecompute.contains(column)) {
+ // Recomputed ancestors keep the provenance emitted by this build. In
particular, a continue-on-error fallback
+ // must not be overwritten with the old trusted fingerprint.
+ if (columnMetadata.getTransformFunctionProvenanceVersion() ==
ColumnMetadata.UNAVAILABLE) {
+
properties.clearProperty(V1Constants.MetadataKeys.Column.getKeyFor(column,
+ V1Constants.MetadataKeys.Column.TRANSFORM_FUNCTION));
+
properties.clearProperty(V1Constants.MetadataKeys.Column.getKeyFor(column,
+ V1Constants.MetadataKeys.Column.TRANSFORM_FUNCTION_BASE64));
+
properties.clearProperty(V1Constants.MetadataKeys.Column.getKeyFor(column,
+
V1Constants.MetadataKeys.Column.TRANSFORM_FUNCTION_PROVENANCE_VERSION));
+
properties.clearProperty(V1Constants.MetadataKeys.Column.getKeyFor(column,
+ V1Constants.MetadataKeys.Column.TRANSFORM_FUNCTION_FINGERPRINT));
+ } else {
+ BaseSegmentCreator.addTransformFunction(properties, column,
columnMetadata.getTransformFunction(),
+ columnMetadata.getTransformFunctionFingerprint());
+ // Preserve the exact provenance version. addTransformFunction()
normally infers the version for new writes,
+ // but an unrelated refresh must not silently upgrade or downgrade
metadata it did not regenerate.
+
properties.setProperty(V1Constants.MetadataKeys.Column.getKeyFor(column,
+
V1Constants.MetadataKeys.Column.TRANSFORM_FUNCTION_PROVENANCE_VERSION),
+ columnMetadata.getTransformFunctionProvenanceVersion());
+ }
+ }
+ }
+ for (String column : replayDefaultColumns) {
+ // Row replay supplies this value before expressions, but the regular
segment-generation path otherwise records
+ // it like a source field. Preserve its default-column identity so a
later structural or transform change can
+ // regenerate it instead of treating the previously materialized default
as authoritative input.
+ properties.setProperty(V1Constants.MetadataKeys.Column.getKeyFor(column,
Review Comment:
A column enters `replayDefaultColumns` when it was absent from the old
segment, even if an enricher supplies its value on every replayed row. This
loop then marks that enriched column auto-generated. On a later schema-default
change, server reload treats it as a default column and replaces its real
values (the new enriched-column test verifies values 15/25 immediately after
refresh, but not the next reload). Please record whether replay actually
supplied defaults, and avoid marking enriched or mixed columns wholly
default-backed.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:
##########
@@ -414,6 +442,19 @@ Map<String, DefaultColumnAction>
computeDefaultColumnActionMap() {
return defaultColumnActionMap;
}
+ /// Returns `true` when a column with known provenance was generated by a
different transform expression than the one
+ /// configured now. Legacy columns have no provenance marker and are left
untouched because their generating
+ /// expression is unknowable. A null expression with known provenance is a
known no-transform state.
+ ///
+ /// The column is then regenerated like any other updated column, in
[#orderByDependencies] order, so a changed
+ /// input is rebuilt before the columns that read it. A dependent whose own
expression did not change is not
+ /// regenerated, which is why metadata written here records the expression
without a dependency fingerprint and stays
+ /// provenance version 1. Dependency-closed version-2 provenance is written
only by full record replay.
+ private boolean isTransformFunctionChanged(String column, ColumnMetadata
columnMetadata) {
Review Comment:
This compares only the column's direct expression. If existing
auto-generated `A` changes from `plus(raw,1)` to `plus(raw,2)` while existing
`B=plus(A,1)` is unchanged, reload updates `A` but gives `B` no action, so `B`
continues serving values computed from old `A`. Dependency ordering only orders
selected actions; it does not select `B`. Please include affected existing
dependents in the rebuild and index-invalidation set.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:
##########
@@ -348,6 +368,8 @@ Map<String, DefaultColumnAction>
computeDefaultColumnActionMap() {
defaultColumnActionMap.put(column,
DefaultColumnAction.UPDATE_DIMENSION_DEFAULT_VALUE);
} else if (isSingleValueInMetadata != isSingleValueInSchema) {
defaultColumnActionMap.put(column,
DefaultColumnAction.UPDATE_DIMENSION_NUMBER_OF_VALUES);
+ } else if (isTransformFunctionChanged(column, columnMetadata)) {
Review Comment:
This new transform UPDATE reaches the V1/V3 handlers, which remove the old
column indexes and metadata before building the replacement. With the default
`errorOnColumnBuildFailure=false`, a valid expression whose result cannot
convert to the column type (for example a nonnumeric string for an INT derived
column) makes creation return `false`; `updateDefaultColumns()` then saves
metadata without the previously valid column. Please build and validate the
replacement before removing the old column, or restore the old state on
tolerated failure.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/ColumnMetadata.java:
##########
@@ -85,6 +85,19 @@ default Map<String, DataType> getSparseKeyTypes() {
return null;
}
+ /// Returns the transform function expression used to generate the column,
if persisted in the segment metadata.
+ @Nullable
+ String getTransformFunction();
Review Comment:
Making these three new public SPI getters abstract breaks existing
downstream `ColumnMetadata` implementations: they no longer compile against
this interface, and an old compiled implementation throws `AbstractMethodError`
if a new getter is called. We have an existing downstream implementation in the
aggregate build that lacks all three methods. Please retain default
implementations returning `null`, `UNAVAILABLE`, and `null`; the in-repo mapped
implementation can still override them explicitly.
--
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]
