egalpin commented on code in PR #10704:
URL: https://github.com/apache/pinot/pull/10704#discussion_r1184223075
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ComparisonColumns.java:
##########
@@ -37,10 +38,43 @@ public int getComparableIndex() {
return _comparableIndex;
}
+ public int compareToSealed(ComparisonColumns other) {
+ /*
+ - iterate over all columns
+ - if any value in _values is greater than its counterpart in
_other._values, keep _values as-is and return 1
+ - if all values in _values are less than those in _other._values, keep
_values as-is and return -1
+ - if all values between the two sets of Comparables are equal
(compareTo == 0), keep _values as-is and return 0
Review Comment:
@Jackie-Jiang typically comparisonResult ≥ 1 would be "truthy" for "persist
the record" but I feel that because "null" values (ie defaultNull values) for
newly added columns could be compared to one another and yield a comparison
result of 0, when we actually don't want to use the comparison result of the
newly added columns. So if we enforce that `0` can only be the result if _all_
columns yield `0` when compared, I think we can achieve the approach of
"maintain the same upsert semantics from before the mutation of
comparisonColumns list in table config".
Thoughts?
--
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]
