egalpin commented on code in PR #10234:
URL: https://github.com/apache/pinot/pull/10234#discussion_r1106523990
##########
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:
As a result of having a setter for both a String or list of String (below
code snippet), Jackson databind will use the correct setter. In this case, a
singular `comparisonColumn` will be coerced to a list, so all existing configs
will continue to work as they did before without the need for any users to
alter their configs. I have a test for that using
[pinot-common/src/test/resources/testConfigs/multipleIndexPartialUpsertConfig.json](https://github.com/apache/pinot/pull/10234/files#diff-d7f3fdcb8570dfc02a7430ebf0ac83051edd37a1bc5c80a2e728fec0122b3206)
```java
public void setComparisonColumn(List<String> comparisonColumns) {
_comparisonColumns = comparisonColumns;
}
public void setComparisonColumn(String comparisonColumn) {
_comparisonColumns = Collections.singletonList(comparisonColumn);
}
```
I think there might be another question around whether or not we want to
do that coercion from String to SingletonList. If we don't coerce, I feel
there will be a lot of conditional logic throughout the codebase. If we do
coerce, there will be some overhead for cases with only one comparisonColumn.
There shouldn't really be any performance impact since list of size 1 will
have O(1) complexity, but there would be memory overhead in the use of
ComparisonColumns class. One thing that's nice about coercing to always have a
list of columns is that it would mean anyone could add a new comparisonColumn
at any time without the need for a server restart
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
