Re: [PR] Add post-partial-upsert transforms for derived columns [pinot]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #17308:
URL: https://github.com/apache/pinot/pull/17308#discussion_r2956178520


##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ExpressionTransformer.java:
##########
@@ -46,15 +53,89 @@ public class ExpressionTransformer implements RecordTransformer {
   private static final Logger LOGGER = LoggerFactory.getLogger(ExpressionTransformer.class);
 
   @VisibleForTesting
-  final LinkedHashMap<String, FunctionEvaluator> _expressionEvaluators;
+  final LinkedHashMap<String, FunctionEvaluator> _expressionEvaluators = new LinkedHashMap<>();
+  private final Set<String> _implicitMapTransformColumns = new HashSet<>();
   private final boolean _continueOnError;
   private final ThrottledLogger _throttledLogger;
+  /**
+   * If {@code true}, transform functions overwrite existing non-null values instead of skipping them. This is enabled
+   * for post-upsert transforms where derived columns should be recomputed on the merged row; otherwise transforms only
+   * populate missing or null-valued columns.
+   */
+  private final boolean _overwriteExistingValues;
 
   public ExpressionTransformer(TableConfig tableConfig, Schema schema) {
-    _expressionEvaluators = ExpressionTransformerUtils.getTopologicallySortedExpressions(tableConfig, schema);
+    this(tableConfig, schema, null, false);
+  }
+
+  public ExpressionTransformer(TableConfig tableConfig, Schema schema,
+      @Nullable List<TransformConfig> transformConfigs, boolean overwriteExistingValues) {
+    Map<String, FunctionEvaluator> expressionEvaluators = new HashMap<>();
     IngestionConfig ingestionConfig = tableConfig.getIngestionConfig();
+    List<TransformConfig> resolvedTransformConfigs =
+        transformConfigs != null ? transformConfigs
+            : ingestionConfig != null ? ingestionConfig.getTransformConfigs() : null;
+    if (resolvedTransformConfigs != null) {
+      for (TransformConfig transformConfig : resolvedTransformConfigs) {
+        FunctionEvaluator previous = expressionEvaluators.put(transformConfig.getColumnName(),
+            FunctionEvaluatorFactory.getExpressionEvaluator(transformConfig.getTransformFunction()));
+        Preconditions.checkState(previous == null,
+            "Cannot set more than one ingestion transform function on column: %s.", transformConfig.getColumnName());

Review Comment:
   The duplicate-transform check error message is now used for both ingestion-time and post-partial-upsert transform configs, but the message hard-codes "ingestion". Consider rewording it to a more general "transform function" message so it’s accurate in both contexts.
   



##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/RecordTransformerUtils.java:
##########
@@ -89,6 +92,37 @@ public static List<RecordTransformer> getDefaultTransformers(TableConfig tableCo
     return getTransformers(tableConfig, schema, false, false, false, false);
   }
 
+  /**
+   * Returns transformers to apply after a partial upsert merge. Only post-merge transform configs are honored to avoid
+   * re-running ingestion-time transforms. Derived columns must exist in the schema to be queryable.
+   *
+   * @param tableConfig The table configuration containing post-partial-upsert transform configs
+   * @param schema The table schema used for validation and type conversion
+   * @return List of transformers to apply after merge, or empty list if none configured
+   */
+  public static List<RecordTransformer> getPostPartialUpsertTransformers(TableConfig tableConfig, Schema schema) {
+    UpsertConfig upsertConfig = tableConfig.getUpsertConfig();
+    if (upsertConfig == null) {
+      return List.of();
+    }
+    List<TransformConfig> postPartialUpsertTransformConfigs = upsertConfig.getPostPartialUpsertTransformConfigs();
+    if (CollectionUtils.isEmpty(postPartialUpsertTransformConfigs)) {
+      return List.of();
+    }
+    List<RecordTransformer> transformers = new ArrayList<>();
+    // TODO: Only re-apply transforms to columns touched by the partial upsert merge to avoid recomputing unrelated
+    //       derived fields.

Review Comment:
   New TODO in production code: either implement the optimization or convert this into a tracked issue/link (and remove the TODO) to align with the repo’s guideline of avoiding TODOs in committed code.
   



-- 
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