xiangfu0 commented on code in PR #18129: URL: https://github.com/apache/pinot/pull/18129#discussion_r3063586840
########## pinot-common/src/main/java/org/apache/pinot/common/filter/FilterPredicateRegistry.java: ########## @@ -0,0 +1,115 @@ +/** + * 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.common.filter; + +import java.util.Collection; +import java.util.Locale; +import java.util.Map; +import java.util.ServiceLoader; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; +import javax.annotation.Nullable; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +/** + * Registry for custom filter predicate plugins. + * + * <p>Plugins are discovered automatically via {@link ServiceLoader} during {@link #init()}, + * and can also be registered programmatically via {@link #register(FilterPredicatePlugin)}. + * + * <p>This registry is consulted at multiple points in the query pipeline when the filter name + * is not a built-in {@code FilterKind}: + * <ul> + * <li>{@code CalciteSqlParser.validateFilter()} — filter validation</li> + * <li>{@code PredicateComparisonRewriter} — expression rewriting</li> + * <li>{@code RequestContextUtils.getFilterInner()} — predicate creation</li> + * <li>{@code PinotOperatorTable} — Calcite SQL operator registration</li> + * </ul> + */ +public class FilterPredicateRegistry { + private static final Logger LOGGER = LoggerFactory.getLogger(FilterPredicateRegistry.class); + private static final Map<String, FilterPredicatePlugin> REGISTRY = new ConcurrentHashMap<>(); + private static final AtomicBoolean INITIALIZED = new AtomicBoolean(false); + + private FilterPredicateRegistry() { + } + + /** + * Discovers and registers all {@link FilterPredicatePlugin} implementations found via ServiceLoader. + * Safe to call multiple times; subsequent calls after the first are no-ops. + */ + public static void init() { + if (!INITIALIZED.compareAndSet(false, true)) { + return; + } + ServiceLoader<FilterPredicatePlugin> loader = ServiceLoader.load(FilterPredicatePlugin.class); Review Comment: This `ServiceLoader.load(...)` only scans the thread-context/system classloader. Pinot plugins loaded from `plugins.dir` go through `PluginManager` and live in isolated plugin realms/classloaders; on Java 11+ the system loader is not a `URLClassLoader`, so those jars are not visible here. In practice that means a standard Pinot plugin jar will never be discovered on broker/controller, and custom predicates won't register unless the implementation is placed on the main application classpath. Can we integrate this with `PluginManager` (or otherwise enumerate plugin classloaders) instead of using the default `ServiceLoader` lookup? ########## pinot-core/src/main/java/org/apache/pinot/core/operator/filter/custom/CustomFilterOperatorRegistry.java: ########## @@ -0,0 +1,102 @@ +/** + * 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.core.operator.filter.custom; + +import java.util.Locale; +import java.util.Map; +import java.util.ServiceLoader; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; +import javax.annotation.Nullable; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +/** + * Registry for custom filter operator factories. + * + * <p>Plugins register a {@link CustomFilterOperatorFactory} that creates filter operators + * for custom predicate types at query execution time. Factories are discovered via + * {@link ServiceLoader} during {@link #init()}, or registered programmatically. + * + * <p>This registry is consulted by {@code FilterPlanNode} when a predicate of type + * {@code CUSTOM} is encountered. + */ +public class CustomFilterOperatorRegistry { + private static final Logger LOGGER = LoggerFactory.getLogger(CustomFilterOperatorRegistry.class); + private static final Map<String, CustomFilterOperatorFactory> REGISTRY = new ConcurrentHashMap<>(); + private static final AtomicBoolean INITIALIZED = new AtomicBoolean(false); + + private CustomFilterOperatorRegistry() { + } + + /** + * Discovers and registers all {@link CustomFilterOperatorFactory} implementations via ServiceLoader. + * Safe to call multiple times; subsequent calls after the first are no-ops. + */ + public static void init() { + if (!INITIALIZED.compareAndSet(false, true)) { + return; + } + ServiceLoader<CustomFilterOperatorFactory> loader = ServiceLoader.load(CustomFilterOperatorFactory.class); Review Comment: Same classloading problem as the predicate registry: this default `ServiceLoader` lookup will not see factories packaged as normal Pinot plugins in `plugins.dir`, because those jars are loaded into plugin-specific classloaders by `PluginManager`, not the thread-context/system loader. That leaves the server unable to find `CustomFilterOperatorFactory` implementations even when the broker successfully parses the predicate, so queries will fail at runtime with `No CustomFilterOperatorFactory registered...` in the standard deployment model. ########## pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java: ########## @@ -321,6 +323,13 @@ private static FilterContext getFilterInner(ExpressionContext filterExpression) private static FilterContext getFilterInner(FunctionContext filterFunction) { String functionOperator = filterFunction.getFunctionName().toUpperCase(); + // Check if this is a custom filter predicate registered via plugin + FilterPredicatePlugin customPlugin = FilterPredicateRegistry.get(functionOperator); Review Comment: Looking up the custom registry before both the built-in `FilterKind` handling and the boolean-function fallback means a plugin named `TEXT_MATCH`, `IN`, or even `startsWith` will silently hijack existing query semantics in `WHERE`. That's a backward-compat break triggered just by dropping a jar on the classpath. The registry needs to reject collisions with built-in predicates/functions, or this lookup needs to happen only after built-ins and normal boolean functions have been ruled out. -- 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]
