Copilot commented on code in PR #18446:
URL: https://github.com/apache/pinot/pull/18446#discussion_r3211472560
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/PartitionFunctionFactory.java:
##########
@@ -18,86 +18,118 @@
*/
package org.apache.pinot.segment.spi.partition;
+import com.google.common.base.Preconditions;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Modifier;
+import java.util.Collections;
import java.util.HashMap;
+import java.util.Locale;
import java.util.Map;
import javax.annotation.Nullable;
import org.apache.pinot.segment.spi.partition.metadata.ColumnPartitionMetadata;
+import org.apache.pinot.spi.annotations.PartitionFunctionType;
import org.apache.pinot.spi.config.table.ColumnPartitionConfig;
+import org.apache.pinot.spi.utils.PinotReflectionUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
- * Factory to build instances of {@link PartitionFunction}.
+ * Dynamic registry for {@link PartitionFunction} implementations.
+ *
+ * <p>Discovery is driven by classpath scanning for classes annotated with
+ * {@link PartitionFunctionType}. Annotated classes must:
+ * <ul>
+ * <li>be public and implement {@link PartitionFunction}</li>
+ * <li>live under a package matching the regex {@code
.*\.partition\.function\..*}
+ * (e.g. {@code org.apache.pinot.common.partition.function} or any
plugin package
+ * that follows the same convention)</li>
+ * <li>expose a public constructor with signature
+ * {@code (int numPartitions, Map<String, String> functionConfig)}</li>
+ * </ul>
+ *
+ * <p>The static block scans the classpath once and builds an immutable
+ * (canonicalized name → constructor) map. Instances are created on demand by
+ * {@link #getPartitionFunction(String, int, Map)}.
+ *
+ * <p>To force eager initialization (e.g. so the scan happens before the first
segment
+ * is read), call {@link #init()} from broker/server/controller startup.
*/
public class PartitionFunctionFactory {
- // Enum for various partition functions to be added.
- public enum PartitionFunctionType {
- Modulo, Murmur, Murmur2, Murmur3, Fnv, ByteArray, HashCode,
BoundedColumnValue;
- // Add more functions here.
+ private PartitionFunctionFactory() {
+ }
- private static final Map<String, PartitionFunctionType> VALUE_MAP = new
HashMap<>();
+ private static final Logger LOGGER =
LoggerFactory.getLogger(PartitionFunctionFactory.class);
+ private static final String SCAN_REGEX = ".*\\.partition\\.function\\..*";
- static {
- for (PartitionFunctionType functionType :
PartitionFunctionType.values()) {
- VALUE_MAP.put(functionType.name().toLowerCase(), functionType);
- }
- }
-
- public static PartitionFunctionType fromString(String name) {
- PartitionFunctionType functionType = VALUE_MAP.get(name.toLowerCase());
+ private static final Map<String, Constructor<? extends PartitionFunction>>
REGISTRY;
- if (functionType == null) {
- throw new IllegalArgumentException("No enum constant for: " + name);
+ static {
+ long startTimeMs = System.currentTimeMillis();
+ Map<String, Constructor<? extends PartitionFunction>> registry = new
HashMap<>();
+ for (Class<?> clazz :
PinotReflectionUtils.getClassesThroughReflection(SCAN_REGEX,
PartitionFunctionType.class)) {
+ if (!Modifier.isPublic(clazz.getModifiers()) ||
!PartitionFunction.class.isAssignableFrom(clazz)) {
Review Comment:
`PartitionFunctionFactory` discovery currently calls
`PinotReflectionUtils.getClassesThroughReflection(SCAN_REGEX, ...)`, which only
scans under the hard-coded base package `org.apache.pinot` (see
`PinotReflectionUtils.PINOT_PACKAGE_NAME`). That means third-party partition
functions in external packages (e.g. `com.acme.partition.function.*`) will
never be discovered, despite the annotation/Javadoc implying they will.
Consider either (a) scanning additional configurable base packages (similar to
other registries that accept a package list), or (b) updating the plugin
contract/Javadoc to explicitly require `org.apache.pinot.*` packages.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/PartitionFunctionFactory.java:
##########
@@ -108,4 +140,8 @@ public static PartitionFunction
getPartitionFunction(ColumnPartitionConfig confi
public static PartitionFunction getPartitionFunction(ColumnPartitionMetadata
metadata) {
return getPartitionFunction(metadata.getFunctionName(),
metadata.getNumPartitions(), metadata.getFunctionConfig());
}
+
+ private static String canonicalize(String name) {
+ return name.toLowerCase(Locale.ROOT);
Review Comment:
The Javadoc for `functionName` says lookup is case-insensitive “after
stripping underscores”, but `canonicalize()` currently only lower-cases the
name. Either implement the underscore-stripping behavior (and consider
trimming) or update the Javadoc to match the actual normalization.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/PartitionFunction.java:
##########
@@ -57,4 +59,19 @@ public interface PartitionFunction extends Serializable {
default Map<String, String> getFunctionConfig() {
return null;
}
+
+ /**
+ * Reports the {@link PartitionIntNormalizer} that drives this partition
function's int-to-id
+ * mapping. The built-in implementations now apply the named normalizer
directly, so the value is
+ * authoritative — recomputing a partition via
+ * {@code
PartitionIntNormalizer.valueOf(getPartitionIdNormalizer()).getPartitionId(rawHash,
numPartitions)}
+ * yields the same result as {@link #getPartition(String)} for the same raw
hash input.
+ *
+ * <p>Used by the framework for identity / staleness matching between
config-side and segment-side
+ * partition metadata. Each implementation must declare its own value —
there is intentionally no
+ * default. Plug-ins whose output is already in {@code [0, numPartitions)}
should return
+ * {@link PartitionIntNormalizer#POSITIVE_MODULO} (a no-op label).
+ */
+ @JsonIgnore
+ String getPartitionIdNormalizer();
Review Comment:
`PartitionFunction` adds `getPartitionIdNormalizer()` as a new abstract
method. Because this is a public SPI interface, making it abstract is
source/binary incompatible for any existing external `PartitionFunction`
implementations (they will fail to compile or can hit `AbstractMethodError` at
runtime). If backward compatibility is desired (as stated in the PR
description), make this a `default` method (e.g., returning
`PartitionIntNormalizer.POSITIVE_MODULO.name()`), and let built-ins override it.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/annotations/PartitionFunctionType.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.spi.annotations;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+
+/**
+ * Marker annotation for plug-in {@code PartitionFunction} implementations.
+ *
+ * <p>Classes annotated with this annotation are auto-discovered at startup by
+ * {@code PartitionFunctionFactory} via classpath scanning. Each annotated
class must:
+ * <ul>
+ * <li>Implement {@code
org.apache.pinot.segment.spi.partition.PartitionFunction}</li>
+ * <li>Be public</li>
+ * <li>Live under a package matching {@code .*\.partition\.function\..*}
(e.g.
+ * {@code org.apache.pinot.common.partition.function} or any plugin
package
+ * that follows the same convention)</li>
Review Comment:
`@PartitionFunctionType` Javadoc states implementations can live in “any
plugin package that follows the same convention”, but the current factory
implementation scans only within `org.apache.pinot` (via
`PinotReflectionUtils.getClassesThroughReflection(...)`). Either adjust
discovery to include non-`org.apache.pinot` packages, or update this Javadoc to
avoid advertising unsupported plugin locations.
--
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]
