egalpin commented on code in PR #10234: URL: https://github.com/apache/pinot/pull/10234#discussion_r1121006314
########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ComparisonColumns.java: ########## @@ -0,0 +1,91 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.segment.local.upsert; + + +@SuppressWarnings({"rawtypes", "unchecked"}) +public class ComparisonColumns implements Comparable<ComparisonColumns> { + private Comparable[] _values; + + public ComparisonColumns(Comparable[] values) { + _values = values; + } + + public Comparable[] getValues() { + return _values; + } + + @Override + public int compareTo(ComparisonColumns other) { + // _comparisonColumns should only at most one non-null comparison value. If not, it is the user's responsibility. + // There is no attempt to guarantee behavior in the case where there are multiple non-null values + int comparisonResult; + int comparableIndex = getComparableIndex(); + + if (comparableIndex < 0) { + // All comparison values were null. This record is only ok to keep if all prior values were also null + comparisonResult = 1; + for (int i = 0; i < other.getValues().length; i++) { + if (other.getValues()[i] != null) { + comparisonResult = -1; + break; + } + } + } else { + Comparable comparisonValue = _values[comparableIndex]; + Comparable otherComparisonValue = other.getValues()[comparableIndex]; + + if (otherComparisonValue == null) { + // Keep this record because the existing record has no value for the same comparison column, therefore the + // (lack of) existing value could not possibly cause the new value to be rejected. + comparisonResult = 1; + } else { + comparisonResult = comparisonValue.compareTo(otherComparisonValue); + } + } + + if (comparisonResult >= 0) { + // TODO(egalpin): This method currently may have side-effects on _values. Depending on the comparison result, + // entities from {@param other} may be merged into _values. This really should not be done implicitly as part + // of compareTo, but has been implemented this way to minimize the changes required within all subclasses of + // {@link BasePartitionUpsertMetadataManager}. Ideally, this merge should only be triggered explicitly by + // implementations of {@link BasePartitionUpsertMetadataManager}. + for (int i = 0; i < _values.length; i++) { + // N.B. null check _must_ be here to prevent overwriting _values[i] with null from other._values[i], such + // as in the case where this is the first time that a non-null value has been received for a given + // comparableIndex after previously receiving non-null value for a different comparableIndex Review Comment: I added this check after my unit tests initially failed. It turned out to be a bug where the comparableIndex could change due to computing it based on first non-null value in _values and the fact that compareTo implicitly changes the contents of _values throughout execution. Fixed in fb25fc9d38 -- 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
