Copilot commented on code in PR #9553: URL: https://github.com/apache/gravitino/pull/9553#discussion_r2652176086
########## core/src/main/java/org/apache/gravitino/catalog/ManagedFunctionOperations.java: ########## @@ -0,0 +1,130 @@ +/* + * 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.gravitino.catalog; + +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.exceptions.FunctionAlreadyExistsException; +import org.apache.gravitino.exceptions.NoSuchFunctionException; +import org.apache.gravitino.exceptions.NoSuchFunctionVersionException; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.function.Function; +import org.apache.gravitino.function.FunctionCatalog; +import org.apache.gravitino.function.FunctionChange; +import org.apache.gravitino.function.FunctionColumn; +import org.apache.gravitino.function.FunctionDefinition; +import org.apache.gravitino.function.FunctionType; +import org.apache.gravitino.rel.types.Type; +import org.apache.gravitino.storage.IdGenerator; + +/** + * {@code ManagedFunctionOperations} provides the storage-level operations for managing functions in + * Gravitino's EntityStore. + * + * <p>This class handles the actual persistence of function metadata, including: + * + * <ul> + * <li>Storing function entities and their versions + * <li>Retrieving functions by identifier or version + * <li>Updating function metadata + * <li>Deleting functions and their versions + * </ul> + */ +public class ManagedFunctionOperations implements FunctionCatalog { + + @SuppressWarnings("UnusedVariable") + private static final int INIT_VERSION = 0; + + @SuppressWarnings("UnusedVariable") + private final EntityStore store; + + @SuppressWarnings("UnusedVariable") + private final IdGenerator idGenerator; + + /** + * Creates a new ManagedFunctionOperations instance. + * + * @param store The EntityStore instance for function persistence. + * @param idGenerator The IdGenerator instance for generating unique IDs. + */ + public ManagedFunctionOperations(EntityStore store, IdGenerator idGenerator) { + this.store = store; + this.idGenerator = idGenerator; + } + + @Override + public NameIdentifier[] listFunctions(Namespace namespace) throws NoSuchSchemaException { + // TODO: Implement when FunctionEntity is available + throw new UnsupportedOperationException("listFunctions: FunctionEntity not yet implemented"); + } + + @Override + public Function getFunction(NameIdentifier ident) throws NoSuchFunctionException { + // TODO: Implement when FunctionEntity is available + throw new UnsupportedOperationException("getFunction: FunctionEntity not yet implemented"); + } + + @Override + public Function getFunction(NameIdentifier ident, int version) + throws NoSuchFunctionException, NoSuchFunctionVersionException { + // TODO: Implement when FunctionEntity is available + throw new UnsupportedOperationException( + "getFunction with version: FunctionEntity not yet implemented"); + } + + @Override + public Function registerFunction( + NameIdentifier ident, + String comment, + FunctionType functionType, + boolean deterministic, + Type returnType, + FunctionDefinition[] definitions) + throws NoSuchSchemaException, FunctionAlreadyExistsException { + // TODO: Implement when FunctionEntity is available + throw new UnsupportedOperationException("registerFunction: FunctionEntity not yet implemented"); + } + + @Override + public Function registerFunction( + NameIdentifier ident, + String comment, + boolean deterministic, + FunctionColumn[] returnColumns, + FunctionDefinition[] definitions) + throws NoSuchSchemaException, FunctionAlreadyExistsException { + // TODO: Implement when FunctionEntity is available + throw new UnsupportedOperationException( + "registerFunction for table-valued functions: FunctionEntity not yet implemented"); + } + + @Override + public Function alterFunction(NameIdentifier ident, FunctionChange... changes) + throws NoSuchFunctionException, IllegalArgumentException { + // TODO: Implement when FunctionEntity is available + throw new UnsupportedOperationException("alterFunction: FunctionEntity not yet implemented"); + } + + @Override + public boolean dropFunction(NameIdentifier ident) { + // TODO: Implement when FunctionEntity is available + throw new UnsupportedOperationException("dropFunction: FunctionEntity not yet implemented"); + } +} Review Comment: All methods in ManagedFunctionOperations throw UnsupportedOperationException with TODO comments. This class is added to the production codebase but is completely non-functional. The new code lacks adequate test coverage for the function management framework. Consider either implementing basic functionality before merging or ensuring there are comprehensive unit tests that demonstrate how this framework will work once FunctionEntity is implemented. ########## api/src/main/java/org/apache/gravitino/exceptions/NoSuchFunctionException.java: ########## @@ -0,0 +1,49 @@ +/* + * 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.gravitino.exceptions; + +import com.google.errorprone.annotations.FormatMethod; +import com.google.errorprone.annotations.FormatString; + +/** Exception thrown when a function with specified name is not existed. */ Review Comment: The comment in line 24 contains a grammatical error. "Exception thrown when a function with specified name is not existed" should be "Exception thrown when a function with the specified name does not exist" for grammatically correct English. ```suggestion /** Exception thrown when a function with the specified name does not exist. */ ``` ########## core/src/main/java/org/apache/gravitino/catalog/ManagedFunctionOperations.java: ########## @@ -0,0 +1,130 @@ +/* + * 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.gravitino.catalog; + +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.exceptions.FunctionAlreadyExistsException; +import org.apache.gravitino.exceptions.NoSuchFunctionException; +import org.apache.gravitino.exceptions.NoSuchFunctionVersionException; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.function.Function; +import org.apache.gravitino.function.FunctionCatalog; +import org.apache.gravitino.function.FunctionChange; +import org.apache.gravitino.function.FunctionColumn; +import org.apache.gravitino.function.FunctionDefinition; +import org.apache.gravitino.function.FunctionType; +import org.apache.gravitino.rel.types.Type; +import org.apache.gravitino.storage.IdGenerator; + +/** + * {@code ManagedFunctionOperations} provides the storage-level operations for managing functions in + * Gravitino's EntityStore. + * + * <p>This class handles the actual persistence of function metadata, including: + * + * <ul> + * <li>Storing function entities and their versions + * <li>Retrieving functions by identifier or version + * <li>Updating function metadata + * <li>Deleting functions and their versions + * </ul> + */ +public class ManagedFunctionOperations implements FunctionCatalog { + + @SuppressWarnings("UnusedVariable") + private static final int INIT_VERSION = 0; + + @SuppressWarnings("UnusedVariable") + private final EntityStore store; + + @SuppressWarnings("UnusedVariable") Review Comment: The fields "store" and "idGenerator" are marked with @SuppressWarnings("UnusedVariable") because they're not yet used in the stub implementation. However, these fields will be essential when the actual implementation is added. Consider removing the suppressions and adding a class-level or constructor comment explaining that these fields are prepared for the upcoming FunctionEntity implementation, rather than suppressing the warnings. ```suggestion * * <p>Note: The {@code store}, {@code idGenerator}, and {@code INIT_VERSION} members are prepared for * the upcoming {@code FunctionEntity}-based implementation and may be unused in this stub class * until that implementation is completed. */ public class ManagedFunctionOperations implements FunctionCatalog { private static final int INIT_VERSION = 0; private final EntityStore store; ``` ########## core/src/main/java/org/apache/gravitino/catalog/FunctionNormalizeDispatcher.java: ########## @@ -0,0 +1,140 @@ +/* + * 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.gravitino.catalog; + +import static org.apache.gravitino.catalog.CapabilityHelpers.applyCapabilities; +import static org.apache.gravitino.catalog.CapabilityHelpers.applyCaseSensitive; +import static org.apache.gravitino.catalog.CapabilityHelpers.getCapability; + +import org.apache.commons.lang3.ArrayUtils; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.connector.capability.Capability; +import org.apache.gravitino.exceptions.FunctionAlreadyExistsException; +import org.apache.gravitino.exceptions.NoSuchFunctionException; +import org.apache.gravitino.exceptions.NoSuchFunctionVersionException; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.function.Function; +import org.apache.gravitino.function.FunctionChange; +import org.apache.gravitino.function.FunctionColumn; +import org.apache.gravitino.function.FunctionDefinition; +import org.apache.gravitino.function.FunctionType; +import org.apache.gravitino.rel.types.Type; + +/** + * {@code FunctionNormalizeDispatcher} normalizes function identifiers and namespaces by applying + * case-sensitivity and other naming capabilities before delegating to the underlying dispatcher. + */ +public class FunctionNormalizeDispatcher implements FunctionDispatcher { + private final CatalogManager catalogManager; + private final FunctionDispatcher dispatcher; + + public FunctionNormalizeDispatcher(FunctionDispatcher dispatcher, CatalogManager catalogManager) { + this.dispatcher = dispatcher; + this.catalogManager = catalogManager; + } + + @Override + public NameIdentifier[] listFunctions(Namespace namespace) throws NoSuchSchemaException { + Namespace caseSensitiveNs = normalizeCaseSensitive(namespace); + NameIdentifier[] identifiers = dispatcher.listFunctions(caseSensitiveNs); + return normalizeCaseSensitive(identifiers); + } + + @Override + public Function getFunction(NameIdentifier ident) throws NoSuchFunctionException { + return dispatcher.getFunction(normalizeCaseSensitive(ident)); + } + + @Override + public Function getFunction(NameIdentifier ident, int version) + throws NoSuchFunctionException, NoSuchFunctionVersionException { + return dispatcher.getFunction(normalizeCaseSensitive(ident), version); + } + + @Override + public boolean functionExists(NameIdentifier ident) { + return dispatcher.functionExists(normalizeCaseSensitive(ident)); + } + + @Override + public Function registerFunction( + NameIdentifier ident, + String comment, + FunctionType functionType, + boolean deterministic, + Type returnType, + FunctionDefinition[] definitions) + throws NoSuchSchemaException, FunctionAlreadyExistsException { + return dispatcher.registerFunction( + normalizeNameIdentifier(ident), + comment, + functionType, + deterministic, + returnType, + definitions); + } + + @Override + public Function registerFunction( + NameIdentifier ident, + String comment, + boolean deterministic, + FunctionColumn[] returnColumns, + FunctionDefinition[] definitions) + throws NoSuchSchemaException, FunctionAlreadyExistsException { + return dispatcher.registerFunction( + normalizeNameIdentifier(ident), comment, deterministic, returnColumns, definitions); + } + + @Override + public Function alterFunction(NameIdentifier ident, FunctionChange... changes) + throws NoSuchFunctionException, IllegalArgumentException { + return dispatcher.alterFunction(normalizeCaseSensitive(ident), changes); + } + + @Override + public boolean dropFunction(NameIdentifier ident) { + return dispatcher.dropFunction(normalizeCaseSensitive(ident)); + } + + private Namespace normalizeCaseSensitive(Namespace namespace) { + Capability capabilities = getCapability(NameIdentifier.of(namespace.levels()), catalogManager); + return applyCaseSensitive(namespace, Capability.Scope.FUNCTION, capabilities); + } + + private NameIdentifier normalizeCaseSensitive(NameIdentifier functionIdent) { + Capability capabilities = getCapability(functionIdent, catalogManager); + return applyCaseSensitive(functionIdent, Capability.Scope.FUNCTION, capabilities); + } + + private NameIdentifier[] normalizeCaseSensitive(NameIdentifier[] functionIdents) { + if (ArrayUtils.isEmpty(functionIdents)) { + return functionIdents; + } + + Capability capabilities = getCapability(functionIdents[0], catalogManager); + return applyCaseSensitive(functionIdents, Capability.Scope.FUNCTION, capabilities); + } + + private NameIdentifier normalizeNameIdentifier(NameIdentifier functionIdent) { + Capability capability = getCapability(functionIdent, catalogManager); + return applyCapabilities(functionIdent, Capability.Scope.FUNCTION, capability); + } +} Review Comment: The FunctionNormalizeDispatcher class lacks test coverage. Following the pattern of other dispatchers in the codebase (like TestTableNormalizeDispatcher and TestFilesetNormalizeDispatcher), there should be unit tests verifying that function identifiers and namespaces are correctly normalized according to catalog capabilities, particularly for case sensitivity handling. ########## api/src/main/java/org/apache/gravitino/function/FunctionColumn.java: ########## @@ -0,0 +1,107 @@ +/* + * 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.gravitino.function; + +import com.google.common.base.Preconditions; +import com.google.common.base.Strings; +import java.util.Objects; +import org.apache.gravitino.annotation.Evolving; +import org.apache.gravitino.rel.types.Type; + +/** Represents a return column of a table-valued function. */ +@Evolving +public class FunctionColumn { + private final String name; + private final Type dataType; + private final String comment; + + private FunctionColumn(String name, Type dataType, String comment) { + Preconditions.checkArgument( + !Strings.isNullOrEmpty(name), "Function column name cannot be null"); Review Comment: The error message "Function column name cannot be null" is inaccurate. The validation checks for both null and empty strings using `Strings.isNullOrEmpty()`, so the message should be "Function column name cannot be null or empty" to match the actual validation logic. ```suggestion !Strings.isNullOrEmpty(name), "Function column name cannot be null or empty"); ``` ########## core/src/main/java/org/apache/gravitino/catalog/ManagedFunctionOperations.java: ########## @@ -0,0 +1,130 @@ +/* + * 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.gravitino.catalog; + +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.exceptions.FunctionAlreadyExistsException; +import org.apache.gravitino.exceptions.NoSuchFunctionException; +import org.apache.gravitino.exceptions.NoSuchFunctionVersionException; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.function.Function; +import org.apache.gravitino.function.FunctionCatalog; +import org.apache.gravitino.function.FunctionChange; +import org.apache.gravitino.function.FunctionColumn; +import org.apache.gravitino.function.FunctionDefinition; +import org.apache.gravitino.function.FunctionType; +import org.apache.gravitino.rel.types.Type; +import org.apache.gravitino.storage.IdGenerator; + +/** + * {@code ManagedFunctionOperations} provides the storage-level operations for managing functions in + * Gravitino's EntityStore. + * + * <p>This class handles the actual persistence of function metadata, including: + * + * <ul> + * <li>Storing function entities and their versions + * <li>Retrieving functions by identifier or version + * <li>Updating function metadata + * <li>Deleting functions and their versions + * </ul> + */ +public class ManagedFunctionOperations implements FunctionCatalog { + + @SuppressWarnings("UnusedVariable") + private static final int INIT_VERSION = 0; + + @SuppressWarnings("UnusedVariable") Review Comment: The INIT_VERSION constant is declared but never used in the class, and is suppressed with @SuppressWarnings("UnusedVariable"). If this constant is intended for future use when FunctionEntity is implemented, add a comment explaining its purpose. Otherwise, consider removing it until it's actually needed. ```suggestion ``` ########## core/src/main/java/org/apache/gravitino/catalog/FunctionOperationDispatcher.java: ########## @@ -0,0 +1,306 @@ +/* + * 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.gravitino.catalog; + +import static org.apache.gravitino.utils.NameIdentifierUtil.getCatalogIdentifier; + +import com.google.common.base.Preconditions; +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.exceptions.FunctionAlreadyExistsException; +import org.apache.gravitino.exceptions.NoSuchFunctionException; +import org.apache.gravitino.exceptions.NoSuchFunctionVersionException; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.function.Function; +import org.apache.gravitino.function.FunctionChange; +import org.apache.gravitino.function.FunctionColumn; +import org.apache.gravitino.function.FunctionDefinition; +import org.apache.gravitino.function.FunctionType; +import org.apache.gravitino.lock.LockType; +import org.apache.gravitino.lock.TreeLockUtils; +import org.apache.gravitino.rel.types.Type; +import org.apache.gravitino.storage.IdGenerator; + +/** + * {@code FunctionOperationDispatcher} is responsible for dispatching function-related operations. + * + * <p>Unlike ModelCatalog which manages its own schemas (via ManagedSchemaOperations), functions are + * registered under schemas managed by the underlying catalog (e.g., Hive, Iceberg). Therefore, this + * dispatcher validates schema existence by calling the underlying catalog's schema operations, then + * delegates actual storage operations to {@link ManagedFunctionOperations}. + */ +public class FunctionOperationDispatcher extends OperationDispatcher implements FunctionDispatcher { + + private final ManagedFunctionOperations managedFunctionOps; + + /** + * Creates a new FunctionOperationDispatcher instance. + * + * @param catalogManager The CatalogManager instance to be used for function operations. + * @param store The EntityStore instance to be used for function operations. + * @param idGenerator The IdGenerator instance to be used for function operations. + */ + public FunctionOperationDispatcher( + CatalogManager catalogManager, EntityStore store, IdGenerator idGenerator) { + super(catalogManager, store, idGenerator); + this.managedFunctionOps = new ManagedFunctionOperations(store, idGenerator); + } + + /** + * List the functions in a namespace from the catalog. + * + * @param namespace A namespace. + * @return An array of function identifiers in the namespace. + * @throws NoSuchSchemaException If the schema does not exist. + */ + @Override + public NameIdentifier[] listFunctions(Namespace namespace) throws NoSuchSchemaException { + NameIdentifier schemaIdent = NameIdentifier.of(namespace.levels()); + NameIdentifier catalogIdent = getCatalogIdentifier(schemaIdent); + + return TreeLockUtils.doWithTreeLock( + schemaIdent, + LockType.READ, + () -> { + // Validate schema exists in the underlying catalog + validateSchemaExists(catalogIdent, schemaIdent); + + // Delegate to managed function operations for storage + return managedFunctionOps.listFunctions(namespace); + }); + } + + /** + * Get a function by {@link NameIdentifier} from the catalog. Returns the latest version. + * + * @param ident A function identifier. + * @return The latest version of the function with the given name. + * @throws NoSuchFunctionException If the function does not exist. + */ + @Override + public Function getFunction(NameIdentifier ident) throws NoSuchFunctionException { + return TreeLockUtils.doWithTreeLock( + ident, + LockType.READ, + () -> { + // Delegate to managed function operations for storage + return managedFunctionOps.getFunction(ident); + }); + } + + /** + * Get a function by {@link NameIdentifier} and version from the catalog. + * + * @param ident A function identifier. + * @param version The function version, counted from 0. + * @return The function with the given name and version. + * @throws NoSuchFunctionException If the function does not exist. + * @throws NoSuchFunctionVersionException If the function version does not exist. + */ + @Override + public Function getFunction(NameIdentifier ident, int version) + throws NoSuchFunctionException, NoSuchFunctionVersionException { + Preconditions.checkArgument(version >= 0, "Function version must be non-negative"); + + return TreeLockUtils.doWithTreeLock( + ident, + LockType.READ, + () -> { + // Delegate to managed function operations for storage + return managedFunctionOps.getFunction(ident, version); + }); + } + + /** + * Register a scalar or aggregate function with one or more definitions (overloads). + * + * @param ident The function identifier. + * @param comment The optional function comment. + * @param functionType The function type. + * @param deterministic Whether the function is deterministic. + * @param returnType The return type. + * @param definitions The function definitions. + * @return The registered function. + * @throws NoSuchSchemaException If the schema does not exist. + * @throws FunctionAlreadyExistsException If the function already exists. + */ + @Override + public Function registerFunction( + NameIdentifier ident, + String comment, + FunctionType functionType, + boolean deterministic, + Type returnType, + FunctionDefinition[] definitions) + throws NoSuchSchemaException, FunctionAlreadyExistsException { + Preconditions.checkArgument( + functionType == FunctionType.SCALAR || functionType == FunctionType.AGGREGATE, + "This method is for scalar or aggregate functions only"); + Preconditions.checkArgument(returnType != null, "Return type is required"); + Preconditions.checkArgument( + definitions != null && definitions.length > 0, "At least one definition is required"); + + validateDefinitions(definitions); + + NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels()); + NameIdentifier catalogIdent = getCatalogIdentifier(ident); + + return TreeLockUtils.doWithTreeLock( + schemaIdent, + LockType.WRITE, + () -> { + // Validate schema exists in the underlying catalog + validateSchemaExists(catalogIdent, schemaIdent); + + // Delegate to managed function operations for storage + return managedFunctionOps.registerFunction( + ident, comment, functionType, deterministic, returnType, definitions); + }); + } + + /** + * Register a table-valued function with one or more definitions (overloads). + * + * @param ident The function identifier. + * @param comment The optional function comment. + * @param deterministic Whether the function is deterministic. + * @param returnColumns The return columns. + * @param definitions The function definitions. + * @return The registered function. + * @throws NoSuchSchemaException If the schema does not exist. + * @throws FunctionAlreadyExistsException If the function already exists. + */ + @Override + public Function registerFunction( + NameIdentifier ident, + String comment, + boolean deterministic, + FunctionColumn[] returnColumns, + FunctionDefinition[] definitions) + throws NoSuchSchemaException, FunctionAlreadyExistsException { + Preconditions.checkArgument( + returnColumns != null && returnColumns.length > 0, + "At least one return column is required for table-valued function"); + Preconditions.checkArgument( + definitions != null && definitions.length > 0, "At least one definition is required"); + + validateDefinitions(definitions); + + NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels()); + NameIdentifier catalogIdent = getCatalogIdentifier(ident); + + return TreeLockUtils.doWithTreeLock( + schemaIdent, + LockType.WRITE, + () -> { + // Validate schema exists in the underlying catalog + validateSchemaExists(catalogIdent, schemaIdent); + + // Delegate to managed function operations for storage + return managedFunctionOps.registerFunction( + ident, comment, deterministic, returnColumns, definitions); + }); + } + + /** + * Applies {@link FunctionChange changes} to a function in the catalog. + * + * @param ident the {@link NameIdentifier} instance of the function to alter. + * @param changes the several {@link FunctionChange} instances to apply to the function. + * @return the updated {@link Function} instance. + * @throws NoSuchFunctionException If the function does not exist. + * @throws IllegalArgumentException If the change is rejected by the implementation. + */ + @Override + public Function alterFunction(NameIdentifier ident, FunctionChange... changes) + throws NoSuchFunctionException, IllegalArgumentException { + Preconditions.checkArgument( + changes != null && changes.length > 0, "At least one change is required"); + + NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels()); + + return TreeLockUtils.doWithTreeLock( + schemaIdent, + LockType.WRITE, + () -> { + // Delegate to managed function operations for storage + return managedFunctionOps.alterFunction(ident, changes); + }); + } + + /** + * Drop a function by name. + * + * @param ident The name identifier of the function. + * @return True if the function is deleted, false if the function does not exist. + */ + @Override + public boolean dropFunction(NameIdentifier ident) { + NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels()); + + return TreeLockUtils.doWithTreeLock( + schemaIdent, + LockType.WRITE, + () -> { + // Delegate to managed function operations for storage + return managedFunctionOps.dropFunction(ident); + }); + } + + /** + * Validates that the schema exists in the underlying catalog. + * + * <p>This is a key difference from ModelCatalog: we check the underlying catalog's schema, not + * Gravitino's internal EntityStore. + * + * @param catalogIdent The catalog identifier. + * @param schemaIdent The schema identifier. + * @throws NoSuchSchemaException If the schema does not exist. + */ + private void validateSchemaExists(NameIdentifier catalogIdent, NameIdentifier schemaIdent) + throws NoSuchSchemaException { + doWithCatalog( + catalogIdent, + c -> { + // Call the underlying catalog's schema operation to validate schema existence + c.doWithSchemaOps(s -> s.loadSchema(schemaIdent)); + return null; + }, + NoSuchSchemaException.class); + } + + /** + * Validates function definitions to ensure no ambiguous arities. + * + * <p>Operations MUST fail if a new definition's invocation arities overlap with existing ones. + * + * @param definitions The function definitions to validate. + * @throws IllegalArgumentException If definitions have overlapping arities. + */ + @SuppressWarnings("UnusedVariable") + private void validateDefinitions(FunctionDefinition[] definitions) { + // TODO: Implement arity overlap validation + // For each definition, calculate the set of possible arities (considering default values) + // Ensure no overlap between definitions + // Example: foo(int, float default 1.0) supports arities (int) and (int, float) + // foo(int, string default 'x') supports arities (int) and (int, string) + // These overlap at arity (int), so should be rejected Review Comment: The validateDefinitions method is marked with @SuppressWarnings("UnusedVariable") and contains only TODO comments without any implementation. This suppression is unnecessary since the parameter "definitions" is explicitly mentioned in the TODO comments. Remove the @SuppressWarnings annotation as there are no actual unused variables once basic validation is implemented. ```suggestion private void validateDefinitions(FunctionDefinition[] definitions) { Preconditions.checkArgument(definitions != null, "definitions must not be null"); for (int i = 0; i < definitions.length; i++) { Preconditions.checkArgument( definitions[i] != null, "definition at index %s must not be null", i); } ``` ########## core/src/main/java/org/apache/gravitino/catalog/FunctionOperationDispatcher.java: ########## @@ -0,0 +1,306 @@ +/* + * 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.gravitino.catalog; + +import static org.apache.gravitino.utils.NameIdentifierUtil.getCatalogIdentifier; + +import com.google.common.base.Preconditions; +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.exceptions.FunctionAlreadyExistsException; +import org.apache.gravitino.exceptions.NoSuchFunctionException; +import org.apache.gravitino.exceptions.NoSuchFunctionVersionException; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.function.Function; +import org.apache.gravitino.function.FunctionChange; +import org.apache.gravitino.function.FunctionColumn; +import org.apache.gravitino.function.FunctionDefinition; +import org.apache.gravitino.function.FunctionType; +import org.apache.gravitino.lock.LockType; +import org.apache.gravitino.lock.TreeLockUtils; +import org.apache.gravitino.rel.types.Type; +import org.apache.gravitino.storage.IdGenerator; + +/** + * {@code FunctionOperationDispatcher} is responsible for dispatching function-related operations. + * + * <p>Unlike ModelCatalog which manages its own schemas (via ManagedSchemaOperations), functions are + * registered under schemas managed by the underlying catalog (e.g., Hive, Iceberg). Therefore, this + * dispatcher validates schema existence by calling the underlying catalog's schema operations, then + * delegates actual storage operations to {@link ManagedFunctionOperations}. + */ +public class FunctionOperationDispatcher extends OperationDispatcher implements FunctionDispatcher { + + private final ManagedFunctionOperations managedFunctionOps; + + /** + * Creates a new FunctionOperationDispatcher instance. + * + * @param catalogManager The CatalogManager instance to be used for function operations. + * @param store The EntityStore instance to be used for function operations. + * @param idGenerator The IdGenerator instance to be used for function operations. + */ + public FunctionOperationDispatcher( + CatalogManager catalogManager, EntityStore store, IdGenerator idGenerator) { + super(catalogManager, store, idGenerator); + this.managedFunctionOps = new ManagedFunctionOperations(store, idGenerator); + } + + /** + * List the functions in a namespace from the catalog. + * + * @param namespace A namespace. + * @return An array of function identifiers in the namespace. + * @throws NoSuchSchemaException If the schema does not exist. + */ + @Override + public NameIdentifier[] listFunctions(Namespace namespace) throws NoSuchSchemaException { + NameIdentifier schemaIdent = NameIdentifier.of(namespace.levels()); + NameIdentifier catalogIdent = getCatalogIdentifier(schemaIdent); + + return TreeLockUtils.doWithTreeLock( + schemaIdent, + LockType.READ, + () -> { + // Validate schema exists in the underlying catalog + validateSchemaExists(catalogIdent, schemaIdent); + + // Delegate to managed function operations for storage + return managedFunctionOps.listFunctions(namespace); + }); + } + + /** + * Get a function by {@link NameIdentifier} from the catalog. Returns the latest version. + * + * @param ident A function identifier. + * @return The latest version of the function with the given name. + * @throws NoSuchFunctionException If the function does not exist. + */ + @Override + public Function getFunction(NameIdentifier ident) throws NoSuchFunctionException { + return TreeLockUtils.doWithTreeLock( + ident, + LockType.READ, + () -> { + // Delegate to managed function operations for storage + return managedFunctionOps.getFunction(ident); + }); + } + + /** + * Get a function by {@link NameIdentifier} and version from the catalog. + * + * @param ident A function identifier. + * @param version The function version, counted from 0. + * @return The function with the given name and version. + * @throws NoSuchFunctionException If the function does not exist. + * @throws NoSuchFunctionVersionException If the function version does not exist. + */ + @Override + public Function getFunction(NameIdentifier ident, int version) + throws NoSuchFunctionException, NoSuchFunctionVersionException { + Preconditions.checkArgument(version >= 0, "Function version must be non-negative"); + + return TreeLockUtils.doWithTreeLock( + ident, + LockType.READ, + () -> { + // Delegate to managed function operations for storage + return managedFunctionOps.getFunction(ident, version); + }); + } + + /** + * Register a scalar or aggregate function with one or more definitions (overloads). + * + * @param ident The function identifier. + * @param comment The optional function comment. + * @param functionType The function type. + * @param deterministic Whether the function is deterministic. + * @param returnType The return type. + * @param definitions The function definitions. + * @return The registered function. + * @throws NoSuchSchemaException If the schema does not exist. + * @throws FunctionAlreadyExistsException If the function already exists. + */ + @Override + public Function registerFunction( + NameIdentifier ident, + String comment, + FunctionType functionType, + boolean deterministic, + Type returnType, + FunctionDefinition[] definitions) + throws NoSuchSchemaException, FunctionAlreadyExistsException { + Preconditions.checkArgument( + functionType == FunctionType.SCALAR || functionType == FunctionType.AGGREGATE, + "This method is for scalar or aggregate functions only"); + Preconditions.checkArgument(returnType != null, "Return type is required"); + Preconditions.checkArgument( + definitions != null && definitions.length > 0, "At least one definition is required"); + + validateDefinitions(definitions); + + NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels()); + NameIdentifier catalogIdent = getCatalogIdentifier(ident); + + return TreeLockUtils.doWithTreeLock( + schemaIdent, + LockType.WRITE, + () -> { + // Validate schema exists in the underlying catalog + validateSchemaExists(catalogIdent, schemaIdent); + + // Delegate to managed function operations for storage + return managedFunctionOps.registerFunction( + ident, comment, functionType, deterministic, returnType, definitions); + }); + } + + /** + * Register a table-valued function with one or more definitions (overloads). + * + * @param ident The function identifier. + * @param comment The optional function comment. + * @param deterministic Whether the function is deterministic. + * @param returnColumns The return columns. + * @param definitions The function definitions. + * @return The registered function. + * @throws NoSuchSchemaException If the schema does not exist. + * @throws FunctionAlreadyExistsException If the function already exists. + */ + @Override + public Function registerFunction( + NameIdentifier ident, + String comment, + boolean deterministic, + FunctionColumn[] returnColumns, + FunctionDefinition[] definitions) + throws NoSuchSchemaException, FunctionAlreadyExistsException { + Preconditions.checkArgument( + returnColumns != null && returnColumns.length > 0, + "At least one return column is required for table-valued function"); + Preconditions.checkArgument( + definitions != null && definitions.length > 0, "At least one definition is required"); + + validateDefinitions(definitions); + + NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels()); + NameIdentifier catalogIdent = getCatalogIdentifier(ident); + + return TreeLockUtils.doWithTreeLock( + schemaIdent, + LockType.WRITE, + () -> { + // Validate schema exists in the underlying catalog + validateSchemaExists(catalogIdent, schemaIdent); + + // Delegate to managed function operations for storage + return managedFunctionOps.registerFunction( + ident, comment, deterministic, returnColumns, definitions); + }); + } + + /** + * Applies {@link FunctionChange changes} to a function in the catalog. + * + * @param ident the {@link NameIdentifier} instance of the function to alter. + * @param changes the several {@link FunctionChange} instances to apply to the function. + * @return the updated {@link Function} instance. + * @throws NoSuchFunctionException If the function does not exist. + * @throws IllegalArgumentException If the change is rejected by the implementation. + */ + @Override + public Function alterFunction(NameIdentifier ident, FunctionChange... changes) + throws NoSuchFunctionException, IllegalArgumentException { + Preconditions.checkArgument( + changes != null && changes.length > 0, "At least one change is required"); + + NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels()); + + return TreeLockUtils.doWithTreeLock( + schemaIdent, + LockType.WRITE, + () -> { + // Delegate to managed function operations for storage + return managedFunctionOps.alterFunction(ident, changes); + }); + } + + /** + * Drop a function by name. + * + * @param ident The name identifier of the function. + * @return True if the function is deleted, false if the function does not exist. + */ + @Override + public boolean dropFunction(NameIdentifier ident) { + NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels()); + + return TreeLockUtils.doWithTreeLock( + schemaIdent, + LockType.WRITE, + () -> { + // Delegate to managed function operations for storage + return managedFunctionOps.dropFunction(ident); + }); + } + + /** + * Validates that the schema exists in the underlying catalog. + * + * <p>This is a key difference from ModelCatalog: we check the underlying catalog's schema, not + * Gravitino's internal EntityStore. + * + * @param catalogIdent The catalog identifier. + * @param schemaIdent The schema identifier. + * @throws NoSuchSchemaException If the schema does not exist. + */ + private void validateSchemaExists(NameIdentifier catalogIdent, NameIdentifier schemaIdent) + throws NoSuchSchemaException { + doWithCatalog( + catalogIdent, + c -> { + // Call the underlying catalog's schema operation to validate schema existence + c.doWithSchemaOps(s -> s.loadSchema(schemaIdent)); + return null; + }, + NoSuchSchemaException.class); + } + + /** + * Validates function definitions to ensure no ambiguous arities. + * + * <p>Operations MUST fail if a new definition's invocation arities overlap with existing ones. + * + * @param definitions The function definitions to validate. + * @throws IllegalArgumentException If definitions have overlapping arities. + */ + @SuppressWarnings("UnusedVariable") + private void validateDefinitions(FunctionDefinition[] definitions) { + // TODO: Implement arity overlap validation + // For each definition, calculate the set of possible arities (considering default values) + // Ensure no overlap between definitions + // Example: foo(int, float default 1.0) supports arities (int) and (int, float) + // foo(int, string default 'x') supports arities (int) and (int, string) + // These overlap at arity (int), so should be rejected + } +} Review Comment: The FunctionOperationDispatcher class lacks comprehensive test coverage. Based on existing patterns in the codebase (like TestModelOperationDispatcher and TestTableOperationDispatcher), this dispatcher should have corresponding unit tests that verify locking behavior, schema validation, precondition checks, and the delegation chain to ManagedFunctionOperations. Tests should cover all public methods even if they currently throw UnsupportedOperationException. ########## api/src/main/java/org/apache/gravitino/function/FunctionParams.java: ########## @@ -0,0 +1,140 @@ +/* + * 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.gravitino.function; + +import com.google.common.base.Preconditions; +import com.google.common.base.Strings; +import java.util.Objects; +import org.apache.gravitino.rel.Column; +import org.apache.gravitino.rel.expressions.Expression; +import org.apache.gravitino.rel.types.Type; + +/** Helper methods to create {@link FunctionParam} instances. */ +public class FunctionParams { + + private FunctionParams() {} + + /** + * Create a {@link FunctionParam} instance. + * + * @param name The parameter name. + * @param dataType The parameter type. + * @return A {@link FunctionParam} instance. + */ + public static FunctionParam of(String name, Type dataType) { + return of(name, dataType, null, Column.DEFAULT_VALUE_NOT_SET); + } + + /** + * Create a {@link FunctionParam} instance with an optional comment. + * + * @param name The parameter name. + * @param dataType The parameter type. + * @param comment The optional comment. + * @return A {@link FunctionParam} instance. + */ + public static FunctionParam of(String name, Type dataType, String comment) { + return of(name, dataType, comment, Column.DEFAULT_VALUE_NOT_SET); + } + + /** + * Create a {@link FunctionParam} instance with an optional comment and default value. + * + * @param name The parameter name. + * @param dataType The parameter type. + * @param comment The optional comment. + * @param defaultValue The optional default value expression. + * @return A {@link FunctionParam} instance. + */ + public static FunctionParam of( + String name, Type dataType, String comment, Expression defaultValue) { + return new FunctionParamImpl(name, dataType, comment, defaultValue); + } + + private static final class FunctionParamImpl implements FunctionParam { + private final String name; + private final Type dataType; + private final String comment; + private final Expression defaultValue; + + private FunctionParamImpl(String name, Type dataType, String comment, Expression defaultValue) { + Preconditions.checkArgument(!Strings.isNullOrEmpty(name), "Parameter name cannot be null"); Review Comment: The error message "Parameter name cannot be null" is inaccurate. The validation checks for both null and empty strings using `Strings.isNullOrEmpty()`, so the message should reflect this. Consider changing it to "Parameter name cannot be null or empty". ```suggestion Preconditions.checkArgument( !Strings.isNullOrEmpty(name), "Parameter name cannot be null or empty"); ``` ########## api/src/main/java/org/apache/gravitino/exceptions/NoSuchFunctionVersionException.java: ########## @@ -0,0 +1,49 @@ +/* + * 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.gravitino.exceptions; + +import com.google.errorprone.annotations.FormatMethod; +import com.google.errorprone.annotations.FormatString; + +/** Exception thrown when a function with specified version is not existed. */ Review Comment: The comment in line 24 contains a grammatical error. "Exception thrown when a function with specified version is not existed" should be "Exception thrown when a function with the specified version does not exist" for grammatically correct English. ```suggestion /** Exception thrown when a function with the specified version does not exist. */ ``` -- 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]
