[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10234: Add support for multiple comparison columns (i.e. one comparison column per producer sinking to a table)

["Jackie-Jiang (via GitHub)" <gi...@apache.org>]

Jackie-Jiang commented on code in PR #10234:
URL: https://github.com/apache/pinot/pull/10234#discussion_r1098061403


##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/UpsertConfig.java:
##########
@@ -51,8 +53,8 @@ public enum Strategy {
   @JsonPropertyDescription("default upsert strategy for partial mode")
   private Strategy _defaultPartialUpsertStrategy;
 
-  @JsonPropertyDescription("Column for upsert comparison, default to time column")
-  private String _comparisonColumn;
+  @JsonPropertyDescription("Columns for upsert comparison, default to time column")
+  private List<String> _comparisonColumns;

Review Comment:
   This is backward incompatible change. We should support both `comparisonColumn` (existing single comparison column) and `comparisonColumns` (new added multiple comparison columns), and only allow one of them to be configured (at least one of them should be `null`, both of them can be `null` and we use time column as the comparison column)



##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/UpsertUtils.java:
##########
@@ -134,6 +137,38 @@ public void close()
     }
   }
 
+  public static class ComparisonColumnReader implements Closeable {

Review Comment:
   Let's make 2 reader classes, one for single column and one for multiple columns to avoid impacting the performance for existing use cases



##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/RecordInfo.java:
##########
@@ -18,19 +18,27 @@
  */
 package org.apache.pinot.segment.local.upsert;
 
+import javax.annotation.Nullable;
 import org.apache.pinot.spi.data.readers.PrimaryKey;
 
 
 @SuppressWarnings("rawtypes")
 public class RecordInfo {
+  // This default exists for backward compatibility, it is not a real virtual column but uses the $-prefix so that
+  // it cannot conflict with a real column
+//  static final String DEFAULT_COMP_COL_KEY = "$comparisonColumn";
   private final PrimaryKey _primaryKey;
   private final int _docId;
   private final Comparable _comparisonValue;
+  private final String _comparisonColumnName;

Review Comment:
   Adding this field will add extra memory footprint to the existing use cases. I think this info can be derived from the `_comparisonValue`, and we should add a special comparator for the new `_comparisonValue`



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