xiangfu0 commented on code in PR #18165: URL: https://github.com/apache/pinot/pull/18165#discussion_r3068841304
########## pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/pipeline/PartitionIntNormalizer.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.spi.partition.pipeline; + +import com.google.common.base.Preconditions; +import java.util.Locale; +import javax.annotation.Nullable; + + +/** + * Normalizes the final INT output from a compiled partition pipeline into a partition id. + */ +public enum PartitionIntNormalizer { + POSITIVE_MODULO { + @Override + int toPartitionId(int value, int numPartitions) { + int partition = value % numPartitions; + return partition < 0 ? partition + numPartitions : partition; + } + + @Override + int toPartitionId(long value, int numPartitions) { + long partition = value % numPartitions; + return (int) (partition < 0 ? partition + numPartitions : partition); + } + }, + ABS { Review Comment: Clarified this in the enum docs: ABS is the post-modulo absolute-remainder normalizer, MASK is the pre-modulo sign-bit mask normalizer, and POSITIVE_MODULO is the post-modulo +numPartitions path. I kept those semantics explicit instead of overloading ABS with both behaviors. ########## pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/pipeline/PartitionIntNormalizer.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.spi.partition.pipeline; + +import com.google.common.base.Preconditions; +import java.util.Locale; +import javax.annotation.Nullable; + + +/** + * Normalizes the final INT output from a compiled partition pipeline into a partition id. + */ +public enum PartitionIntNormalizer { Review Comment: Addressed in the latest push. Legacy partition functions now expose their effective built-in normalizer via getPartitionIdNormalizer(), and there is regression coverage for those defaults in PartitionFunctionTest. I am not widening legacy partitionIdNormalizer overrides in this PR because that would change the legacy config contract beyond surfacing the existing behavior. ########## pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/metadata/ColumnPartitionMetadata.java: ########## @@ -92,15 +124,17 @@ public boolean equals(Object obj) { if (obj instanceof ColumnPartitionMetadata) { ColumnPartitionMetadata that = (ColumnPartitionMetadata) obj; return _functionName.equals(that._functionName) && _numPartitions == that._numPartitions && _partitions.equals( - that._partitions) && Objects.equals(_functionConfig, that._functionConfig); + that._partitions) && Objects.equals(_functionConfig, that._functionConfig) && Objects.equals(_functionExpr, + that._functionExpr) && Objects.equals(_partitionIdNormalizer, that._partitionIdNormalizer); } return false; } @Override public int hashCode() { return 37 * 37 * _functionName.hashCode() + 37 * _numPartitions + _partitions.hashCode() - + Objects.hashCode(_functionConfig); + + Objects.hashCode(_functionConfig) + Objects.hashCode(_functionExpr) Review Comment: I kept value equality on ColumnPartitionMetadata. This type represents both the partition function identity and the segment-level partition set, so moving equality onto PartitionFunction would lose the segment-specific partitions dimension. -- 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]
