Copilot commented on code in PR #9551: URL: https://github.com/apache/gravitino/pull/9551#discussion_r2652089082
########## 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 error message uses the term "existed" which is grammatically incorrect. It should be "does not exist" instead of "is not existed". ```suggestion /** Exception thrown when a function with the specified version does not exist. */ ``` ########## api/src/main/java/org/apache/gravitino/function/SQLImpl.java: ########## @@ -0,0 +1,84 @@ +/* + * 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 java.util.Map; +import java.util.Objects; + +/** SQL implementation with runtime and SQL body. */ +public class SQLImpl extends FunctionImpl { + private final String sql; + + SQLImpl( + RuntimeType runtime, + String sql, + FunctionResources resources, + Map<String, String> properties) { + super(Language.SQL, runtime, resources, properties); + this.sql = Preconditions.checkNotNull(sql, "SQL text cannot be null"); + } + + /** + * @return The SQL body. + */ + public String sql() { + return sql; + } + Review Comment: The javadoc comment describes this as the "SQL text body", but the method name is just "sql()". For consistency with other implementation classes that have more descriptive method names (e.g., "className()" for JavaImpl, "handler()" for PythonImpl), consider naming this method "sqlBody()" or "body()" to be more explicit. Additionally, the javadoc could be improved to clarify what this SQL represents (e.g., "The SQL body that implements this function"). ```suggestion * The SQL body that implements this function. * * @return The SQL text body of this function. */ public String sqlBody() { return sql; } /** * Returns the SQL body of this function. * * @return The SQL text body of this function. * @deprecated Use {@link #sqlBody()} instead. */ @Deprecated public String sql() { return sqlBody(); } ``` ########## api/src/main/java/org/apache/gravitino/function/FunctionChange.java: ########## @@ -0,0 +1,392 @@ +/* + * 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 java.util.Arrays; +import java.util.Objects; +import org.apache.gravitino.annotation.Evolving; + +/** Represents a change that can be applied to a function. */ +@Evolving +public interface FunctionChange { + + /** + * Create a {@link FunctionChange} to update the comment of a function. + * + * @param newComment The new comment value. + * @return The change instance. + */ + static FunctionChange updateComment(String newComment) { + return new UpdateComment(newComment); + } + + /** + * Create a {@link FunctionChange} to add a new definition (overload) to a function. + * + * @param definition The new definition to add. + * @return The change instance. + */ + static FunctionChange addDefinition(FunctionDefinition definition) { + return new AddDefinition(definition); + } + + /** + * Create a {@link FunctionChange} to remove an existing definition (overload) from a function. + * + * @param parameters The parameters that identify the definition to remove. + * @return The change instance. + */ + static FunctionChange removeDefinition(FunctionParam[] parameters) { + return new RemoveDefinition(parameters); + } + + /** + * Create a {@link FunctionChange} to add an implementation to a specific definition. + * + * @param parameters The parameters that identify the definition to update. + * @param implementation The implementation to add. + * @return The change instance. + */ + static FunctionChange addImpl(FunctionParam[] parameters, FunctionImpl implementation) { + return new AddImpl(parameters, implementation); + } + + /** + * Create a {@link FunctionChange} to update an implementation for a specific definition and + * runtime. + * + * @param parameters The parameters that identify the definition to update. + * @param runtime The runtime that identifies the implementation to replace. + * @param implementation The new implementation. + * @return The change instance. + */ + static FunctionChange updateImpl( + FunctionParam[] parameters, FunctionImpl.RuntimeType runtime, FunctionImpl implementation) { + return new UpdateImpl(parameters, runtime, implementation); + } + + /** + * Create a {@link FunctionChange} to remove an implementation for a specific definition and + * runtime. + * + * @param parameters The parameters that identify the definition to update. + * @param runtime The runtime that identifies the implementation to remove. + * @return The change instance. + */ + static FunctionChange removeImpl(FunctionParam[] parameters, FunctionImpl.RuntimeType runtime) { + return new RemoveImpl(parameters, runtime); + } + + /** A {@link FunctionChange} to update the comment of a function. */ + final class UpdateComment implements FunctionChange { + private final String newComment; + + UpdateComment(String newComment) { + this.newComment = newComment; + } + + /** + * @return The new comment of the function. + */ + public String newComment() { + return newComment; + } Review Comment: The newComment field is not validated and can be null, but there's no documentation or @Nullable annotation indicating whether null is an acceptable value. If null is allowed, add @Nullable annotation to the newComment() getter and document the semantics (e.g., does null mean "remove comment" or is it invalid?). If null is not allowed, add a Preconditions check in the constructor similar to other change classes. ########## api/src/main/java/org/apache/gravitino/function/FunctionDefinitions.java: ########## @@ -0,0 +1,97 @@ +/* + * 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 java.util.Arrays; + +/** Helper methods to create {@link FunctionDefinition} instances. */ +public final class FunctionDefinitions { + + private FunctionDefinitions() {} + + /** + * Create a {@link FunctionDefinition} instance. + * + * @param parameters The parameters for this definition, maybe empty but not null. + * @param impls The implementations for this definition, it must not be null or empty. + * @return A {@link FunctionDefinition} instance. + */ + public static FunctionDefinition of(FunctionParam[] parameters, FunctionImpl[] impls) { Review Comment: The javadoc comment says "maybe empty but not null" for parameters, but the actual implementation allows null and converts it to an empty array. This inconsistency between documentation and implementation should be corrected. Either update the javadoc to say "may be null or empty", or reject null parameters with a precondition check to match the documented behavior. ########## 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 validation message says "Function column name cannot be null" but the validation checks for both null and empty strings using Strings.isNullOrEmpty(). The error message should accurately reflect that empty strings are also not allowed, such as "Function column name cannot be null or empty". ```suggestion !Strings.isNullOrEmpty(name), "Function column name cannot be null or empty"); ``` ########## 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 parameter name validation checks for null or empty, but the error message says "Parameter name cannot be null" which is incomplete. The message should reflect that empty strings are also not allowed, such as "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/function/PythonImpl.java: ########## @@ -0,0 +1,98 @@ +/* + * 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 java.util.Map; +import java.util.Objects; + +/** Python implementation with handler and optional inline code. */ +public class PythonImpl extends FunctionImpl { + private final String handler; + private final String codeBlock; + + PythonImpl( + RuntimeType runtime, + String handler, + String codeBlock, + FunctionResources resources, + Map<String, String> properties) { + super(Language.PYTHON, runtime, resources, properties); + this.handler = Preconditions.checkNotNull(handler, "Python handler cannot be null"); + this.codeBlock = codeBlock; + } + + /** + * @return The handler entrypoint. + */ + public String handler() { + return handler; + } + + /** + * @return The optional inline code block. + */ + public String codeBlock() { + return codeBlock; + } Review Comment: The codeBlock field can be null (as indicated by line 38 assignment and line 51 javadoc saying "optional"), but the getter method lacks a @Nullable annotation. For consistency with project practices and to help static analysis tools, add @Nullable annotation to this method's return type. ########## api/src/main/java/org/apache/gravitino/function/FunctionCatalog.java: ########## @@ -0,0 +1,147 @@ +/* + * 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 org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.annotation.Evolving; +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.rel.types.Type; + +/** The FunctionCatalog interface defines the public API for managing functions in a schema. */ +@Evolving +public interface FunctionCatalog { + + /** + * 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. + */ + NameIdentifier[] listFunctions(Namespace namespace) throws NoSuchSchemaException; + + /** + * Get a function by {@link NameIdentifier} from the catalog. The identifier only contains the + * schema and function name. A function may include multiple definitions (overloads) in the + * result. This method returns the latest version of the function. + * + * @param ident A function identifier. + * @return The latest version of the function with the given name. + * @throws NoSuchFunctionException If the function does not exist. + */ + Function getFunction(NameIdentifier ident) throws NoSuchFunctionException; + + /** + * Get a function by {@link NameIdentifier} and version from the catalog. The identifier only + * contains the schema and function name. A function may include multiple definitions (overloads) + * in the result. + * + * @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. + */ + Function getFunction(NameIdentifier ident, int version) + throws NoSuchFunctionException, NoSuchFunctionVersionException; + + /** + * Check if a function with the given name exists in the catalog. + * + * @param ident The function identifier. + * @return True if the function exists, false otherwise. + */ + default boolean functionExists(NameIdentifier ident) { + try { + getFunction(ident); + return true; + } catch (NoSuchFunctionException e) { + return false; + } + } + + /** + * 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. + */ + Function registerFunction( + NameIdentifier ident, + String comment, + FunctionType functionType, + boolean deterministic, + Type returnType, + FunctionDefinition[] definitions) + throws NoSuchSchemaException, FunctionAlreadyExistsException; Review Comment: The javadoc states "The function definitions" but doesn't specify constraints on the array. Should document whether the definitions array can be null or empty, and what happens in those cases. Given that a function needs at least one definition to be useful, consider adding validation to require at least one definition, similar to how FunctionDefinitions requires at least one implementation. ########## 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"); + this.name = name; + this.dataType = Preconditions.checkNotNull(dataType, "Parameter data type cannot be null"); + this.comment = comment; + this.defaultValue = defaultValue == null ? Column.DEFAULT_VALUE_NOT_SET : defaultValue; + } + + @Override + public String name() { + return name; + } + + @Override + public Type dataType() { + return dataType; + } + + @Override + public String comment() { + return comment; + } Review Comment: The comment parameter can be null based on the factory methods (line 40-42, 52-54), but the method lacks a @Nullable annotation. For consistency with the project's null-safety practices and to help static analysis tools, add @Nullable annotation to this method's return type. ########## api/src/main/java/org/apache/gravitino/function/FunctionChange.java: ########## @@ -0,0 +1,392 @@ +/* + * 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 java.util.Arrays; +import java.util.Objects; +import org.apache.gravitino.annotation.Evolving; + +/** Represents a change that can be applied to a function. */ +@Evolving +public interface FunctionChange { + + /** + * Create a {@link FunctionChange} to update the comment of a function. + * + * @param newComment The new comment value. + * @return The change instance. + */ + static FunctionChange updateComment(String newComment) { + return new UpdateComment(newComment); + } + + /** + * Create a {@link FunctionChange} to add a new definition (overload) to a function. + * + * @param definition The new definition to add. + * @return The change instance. + */ + static FunctionChange addDefinition(FunctionDefinition definition) { + return new AddDefinition(definition); + } + + /** + * Create a {@link FunctionChange} to remove an existing definition (overload) from a function. + * + * @param parameters The parameters that identify the definition to remove. + * @return The change instance. + */ + static FunctionChange removeDefinition(FunctionParam[] parameters) { + return new RemoveDefinition(parameters); + } + + /** + * Create a {@link FunctionChange} to add an implementation to a specific definition. + * + * @param parameters The parameters that identify the definition to update. + * @param implementation The implementation to add. + * @return The change instance. + */ + static FunctionChange addImpl(FunctionParam[] parameters, FunctionImpl implementation) { + return new AddImpl(parameters, implementation); + } + + /** + * Create a {@link FunctionChange} to update an implementation for a specific definition and + * runtime. + * + * @param parameters The parameters that identify the definition to update. + * @param runtime The runtime that identifies the implementation to replace. + * @param implementation The new implementation. + * @return The change instance. + */ + static FunctionChange updateImpl( + FunctionParam[] parameters, FunctionImpl.RuntimeType runtime, FunctionImpl implementation) { + return new UpdateImpl(parameters, runtime, implementation); + } + + /** + * Create a {@link FunctionChange} to remove an implementation for a specific definition and + * runtime. + * + * @param parameters The parameters that identify the definition to update. + * @param runtime The runtime that identifies the implementation to remove. + * @return The change instance. + */ + static FunctionChange removeImpl(FunctionParam[] parameters, FunctionImpl.RuntimeType runtime) { + return new RemoveImpl(parameters, runtime); + } + + /** A {@link FunctionChange} to update the comment of a function. */ + final class UpdateComment implements FunctionChange { + private final String newComment; + + UpdateComment(String newComment) { + this.newComment = newComment; + } + + /** + * @return The new comment of the function. + */ + public String newComment() { + return newComment; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (!(obj instanceof UpdateComment)) { + return false; + } + UpdateComment that = (UpdateComment) obj; + return Objects.equals(newComment, that.newComment); + } + + @Override + public int hashCode() { + return Objects.hash(newComment); + } + + @Override + public String toString() { + return "UpdateComment{newComment='" + newComment + "'}"; + } + } + + /** A {@link FunctionChange} to add a new definition to a function. */ + final class AddDefinition implements FunctionChange { + private final FunctionDefinition definition; + + AddDefinition(FunctionDefinition definition) { + this.definition = Preconditions.checkNotNull(definition, "Definition cannot be null"); + } + + /** + * @return The definition to add. + */ + public FunctionDefinition definition() { + return definition; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (!(obj instanceof AddDefinition)) { + return false; + } + AddDefinition that = (AddDefinition) obj; + return Objects.equals(definition, that.definition); + } + + @Override + public int hashCode() { + return Objects.hash(definition); + } + + @Override + public String toString() { + return "AddDefinition{definition=" + definition + '}'; + } + } + + /** A {@link FunctionChange} to remove an existing definition from a function. */ + final class RemoveDefinition implements FunctionChange { + private final FunctionParam[] parameters; + + RemoveDefinition(FunctionParam[] parameters) { + Preconditions.checkArgument(parameters != null, "Parameters cannot be null"); + this.parameters = Arrays.copyOf(parameters, parameters.length); + } + + /** + * @return The parameters that identify the definition to remove. + */ + public FunctionParam[] parameters() { + return Arrays.copyOf(parameters, parameters.length); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (!(obj instanceof RemoveDefinition)) { + return false; + } + RemoveDefinition that = (RemoveDefinition) obj; + return Arrays.equals(parameters, that.parameters); + } + + @Override + public int hashCode() { + return Arrays.hashCode(parameters); + } + + @Override + public String toString() { + return "RemoveDefinition{parameters=" + Arrays.toString(parameters) + '}'; + } + } + + /** A {@link FunctionChange} to add an implementation to a definition. */ + final class AddImpl implements FunctionChange { + private final FunctionParam[] parameters; + private final FunctionImpl implementation; + + AddImpl(FunctionParam[] parameters, FunctionImpl implementation) { + Preconditions.checkArgument(parameters != null, "Parameters cannot be null"); + this.parameters = Arrays.copyOf(parameters, parameters.length); + this.implementation = + Preconditions.checkNotNull(implementation, "Implementation cannot be null"); + } + + /** + * @return The parameters that identify the definition to update. + */ + public FunctionParam[] parameters() { + return Arrays.copyOf(parameters, parameters.length); + } + + /** + * @return The implementation to add. + */ + public FunctionImpl implementation() { + return implementation; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (!(obj instanceof AddImpl)) { + return false; + } + AddImpl that = (AddImpl) obj; + return Arrays.equals(parameters, that.parameters) + && Objects.equals(implementation, that.implementation); + } + + @Override + public int hashCode() { + int result = Arrays.hashCode(parameters); + result = 31 * result + Objects.hashCode(implementation); + return result; + } + + @Override + public String toString() { + return "AddImpl{parameters=" + + Arrays.toString(parameters) + + ", implementation=" + + implementation + + '}'; + } + } + + /** + * A {@link FunctionChange} to replace an implementation (identified by runtime) for a specific + * definition. + */ + final class UpdateImpl implements FunctionChange { + private final FunctionParam[] parameters; + private final FunctionImpl.RuntimeType runtime; + private final FunctionImpl implementation; + + UpdateImpl( + FunctionParam[] parameters, FunctionImpl.RuntimeType runtime, FunctionImpl implementation) { + Preconditions.checkArgument(parameters != null, "Parameters cannot be null"); + this.parameters = Arrays.copyOf(parameters, parameters.length); + this.runtime = Preconditions.checkNotNull(runtime, "Runtime cannot be null"); + this.implementation = + Preconditions.checkNotNull(implementation, "Implementation cannot be null"); + Preconditions.checkArgument( + runtime == implementation.runtime(), + "Runtime of implementation must match the runtime being updated"); + } + + /** + * @return The parameters that identify the definition to update. + */ + public FunctionParam[] parameters() { + return Arrays.copyOf(parameters, parameters.length); + } + + /** + * @return The runtime that identifies the implementation to replace. + */ + public FunctionImpl.RuntimeType runtime() { + return runtime; + } + + /** + * @return The new implementation. + */ + public FunctionImpl implementation() { + return implementation; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (!(obj instanceof UpdateImpl)) { + return false; + } + UpdateImpl that = (UpdateImpl) obj; + return Arrays.equals(parameters, that.parameters) + && runtime == that.runtime + && Objects.equals(implementation, that.implementation); + } + + @Override + public int hashCode() { + int result = Arrays.hashCode(parameters); + result = 31 * result + Objects.hash(runtime, implementation); + return result; + } + + @Override + public String toString() { + return "UpdateImpl{parameters=" + + Arrays.toString(parameters) + + ", runtime=" + + runtime + + ", implementation=" + + implementation + + '}'; + } + } + + /** A {@link FunctionChange} to remove an implementation for a specific runtime. */ + final class RemoveImpl implements FunctionChange { + private final FunctionParam[] parameters; + private final FunctionImpl.RuntimeType runtime; + + RemoveImpl(FunctionParam[] parameters, FunctionImpl.RuntimeType runtime) { + Preconditions.checkArgument(parameters != null, "Parameters cannot be null"); + this.parameters = Arrays.copyOf(parameters, parameters.length); + this.runtime = Preconditions.checkNotNull(runtime, "Runtime cannot be null"); + } + + /** + * @return The parameters that identify the definition to update. + */ + public FunctionParam[] parameters() { + return Arrays.copyOf(parameters, parameters.length); + } + + /** + * @return The runtime that identifies the implementation to remove. + */ + public FunctionImpl.RuntimeType runtime() { + return runtime; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (!(obj instanceof RemoveImpl)) { + return false; + } + RemoveImpl that = (RemoveImpl) obj; + return Arrays.equals(parameters, that.parameters) && runtime == that.runtime; + } + + @Override + public int hashCode() { + int result = Arrays.hashCode(parameters); + result = 31 * result + Objects.hashCode(runtime); + return result; + } + + @Override + public String toString() { + return "RemoveImpl{parameters=" + Arrays.toString(parameters) + ", runtime=" + runtime + '}'; + } + } +} Review Comment: All new public API classes and interfaces in this PR lack test coverage. The api module has comprehensive test coverage for similar APIs like TableChange, SchemaChange, MetaLakeChange, etc. Unit tests should be added to verify the behavior of FunctionChange operations, FunctionImpl factory methods, FunctionParams, FunctionDefinitions, FunctionResources, and all the related classes to ensure correctness and maintain the project's test coverage standards. ########## 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"); + this.name = name; + this.dataType = Preconditions.checkNotNull(dataType, "Parameter data type cannot be null"); + this.comment = comment; + this.defaultValue = defaultValue == null ? Column.DEFAULT_VALUE_NOT_SET : defaultValue; + } + + @Override + public String name() { + return name; + } + + @Override + public Type dataType() { + return dataType; + } + + @Override + public String comment() { + return comment; + } + + @Override + public Expression defaultValue() { + return defaultValue; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (!(obj instanceof FunctionParamImpl)) { + return false; + } + FunctionParamImpl that = (FunctionParamImpl) obj; + return Objects.equals(name, that.name) + && Objects.equals(dataType, that.dataType) + && Objects.equals(comment, that.comment) + && Objects.equals(defaultValue, that.defaultValue); Review Comment: The equals method only checks for FunctionParamImpl instances, which means it won't correctly compare with other implementations of the FunctionParam interface. This violates the Liskov Substitution Principle and could cause issues when comparing parameters from different sources. The equals check should be against the FunctionParam interface type and compare using the interface methods (name(), dataType(), comment(), defaultValue()) to ensure compatibility with all implementations. ```suggestion if (!(obj instanceof FunctionParam)) { return false; } FunctionParam that = (FunctionParam) obj; return Objects.equals(name, that.name()) && Objects.equals(dataType, that.dataType()) && Objects.equals(comment, that.comment()) && Objects.equals(defaultValue, that.defaultValue()); ``` ########## api/src/main/java/org/apache/gravitino/function/FunctionImpl.java: ########## @@ -0,0 +1,208 @@ +/* + * 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.collect.ImmutableMap; +import java.util.Map; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.annotation.Evolving; + +/** + * Base class of function implementations. + * + * <p>A function implementation must declare its language and optional external resources. Concrete + * implementations are provided by {@link SQLImpl}, {@link JavaImpl}, and {@link PythonImpl}. + */ +@Evolving +public abstract class FunctionImpl { + /** Supported implementation languages. */ + public enum Language { + /** SQL implementation. */ + SQL, + /** Java implementation. */ + JAVA, + /** Python implementation. */ + PYTHON + } + + /** Supported execution runtimes for function implementations. */ + public enum RuntimeType { + /** Spark runtime. */ + SPARK, + /** Trino runtime. */ + TRINO; + + /** + * Parse a runtime value from string. + * + * @param value Runtime name. + * @return Parsed runtime. + * @throws IllegalArgumentException If the runtime is not supported. + */ + public static RuntimeType fromString(String value) { + Preconditions.checkArgument(StringUtils.isNotBlank(value), "Function runtime must be set"); + try { + return RuntimeType.valueOf(value.trim().toUpperCase()); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("Unsupported function runtime: " + value, e); Review Comment: The exception handling wraps IllegalArgumentException from valueOf() and throws another IllegalArgumentException with the same exception type, which could lead to confusion in error handling. Consider using a more specific exception type or at minimum ensure that the root cause message is preserved. Also, the error message "Unsupported function runtime: " + value should list the supported runtimes to be more helpful to users. ########## api/src/main/java/org/apache/gravitino/function/FunctionResources.java: ########## @@ -0,0 +1,119 @@ +/* + * 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 java.util.Arrays; +import java.util.Objects; +import org.apache.gravitino.annotation.Evolving; + +/** Represents external resources that are required by a function implementation. */ +@Evolving +public class FunctionResources { + private static final String[] EMPTY = new String[0]; + private static final FunctionResources EMPTY_RESOURCES = + new FunctionResources(EMPTY, EMPTY, EMPTY); + + private final String[] jars; + private final String[] files; + private final String[] archives; + + private FunctionResources(String[] jars, String[] files, String[] archives) { + this.jars = jars == null ? EMPTY : Arrays.copyOf(jars, jars.length); + this.files = files == null ? EMPTY : Arrays.copyOf(files, files.length); + this.archives = archives == null ? EMPTY : Arrays.copyOf(archives, archives.length); + } + + /** + * @return An empty {@link FunctionResources} instance. + */ + public static FunctionResources empty() { + return EMPTY_RESOURCES; + } + + /** + * Create a {@link FunctionResources} instance. + * + * @param jars The jar resources. + * @param files The file resources. + * @param archives The archive resources. + * @return A {@link FunctionResources} instance. + */ + public static FunctionResources of(String[] jars, String[] files, String[] archives) { + if ((jars == null || jars.length == 0) + && (files == null || files.length == 0) + && (archives == null || archives.length == 0)) { + return EMPTY_RESOURCES; + } + return new FunctionResources(jars, files, archives); + } + + /** + * @return The jar resources. + */ + public String[] jars() { + return Arrays.copyOf(jars, jars.length); + } + + /** + * @return The file resources. + */ + public String[] files() { + return Arrays.copyOf(files, files.length); + } + + /** + * @return The archive resources. + */ + public String[] archives() { + return Arrays.copyOf(archives, archives.length); Review Comment: The getter methods return defensive copies of the arrays to prevent external mutation. However, the EMPTY constant uses the same EMPTY array instance that is returned by these getters. If a caller modifies the returned array from empty(), it could inadvertently modify the EMPTY_RESOURCES singleton. Consider either documenting that the returned arrays must not be modified, or ensuring the EMPTY_RESOURCES singleton returns a new empty array each time. ########## api/src/main/java/org/apache/gravitino/function/Function.java: ########## @@ -0,0 +1,82 @@ +/* + * 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 javax.annotation.Nullable; +import org.apache.gravitino.Auditable; +import org.apache.gravitino.annotation.Evolving; +import org.apache.gravitino.rel.types.Type; + +/** Represents a user-defined function registered in Gravitino. */ +@Evolving +public interface Function extends Auditable { + + /** + * @return The function name. + */ + String name(); + + /** + * @return The function type. + */ + FunctionType functionType(); + + /** + * @return Whether the function is deterministic. + */ + boolean deterministic(); + + /** + * @return The optional comment of the function. + */ + @Nullable + default String comment() { + return null; + } + + /** + * The return type for scalar or aggregate functions. + * + * @return The return type, null if this is a table-valued function. + */ + @Nullable + default Type returnType() { + return null; + } + + /** + * The output columns for a table-valued function. + * + * @return The output columns of a table-valued function, or an empty array for scalar or + * aggregate functions. + */ + default FunctionColumn[] returnColumns() { + return new FunctionColumn[0]; + } + + /** + * @return The definitions of the function. + */ + FunctionDefinition[] definitions(); + + /** + * @return The version of the function, counted from 0 and incrementing on each alteration. Review Comment: The javadoc states that the version is "counted from 0 and incrementing on each alteration", but this is inconsistent with typical software versioning conventions where version 1 represents the first version. This 0-based versioning should be clarified or reconsidered. If 0-based versioning is intentional, ensure consistent documentation across all related methods (getFunction with version parameter, FunctionCatalog interface). ```suggestion * Returns the internal revision version of the function. * * <p>This version is a 0-based counter, where {@code 0} represents the initial definition of * the function, and the value is incremented by 1 on each subsequent alteration. This is an * internal revision number and does not follow semantic versioning conventions. * * @return The 0-based revision version of the function. ``` ########## 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 error message uses the term "existed" which is grammatically incorrect. It should be "does not exist" instead of "is not existed". ```suggestion /** Exception thrown when a function with the specified name does not exist. */ ``` ########## api/src/main/java/org/apache/gravitino/function/FunctionCatalog.java: ########## @@ -0,0 +1,147 @@ +/* + * 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 org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.annotation.Evolving; +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.rel.types.Type; + +/** The FunctionCatalog interface defines the public API for managing functions in a schema. */ +@Evolving +public interface FunctionCatalog { + + /** + * 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. + */ + NameIdentifier[] listFunctions(Namespace namespace) throws NoSuchSchemaException; + + /** + * Get a function by {@link NameIdentifier} from the catalog. The identifier only contains the + * schema and function name. A function may include multiple definitions (overloads) in the + * result. This method returns the latest version of the function. + * + * @param ident A function identifier. + * @return The latest version of the function with the given name. + * @throws NoSuchFunctionException If the function does not exist. + */ + Function getFunction(NameIdentifier ident) throws NoSuchFunctionException; + + /** + * Get a function by {@link NameIdentifier} and version from the catalog. The identifier only + * contains the schema and function name. A function may include multiple definitions (overloads) + * in the result. + * + * @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. + */ + Function getFunction(NameIdentifier ident, int version) + throws NoSuchFunctionException, NoSuchFunctionVersionException; + + /** + * Check if a function with the given name exists in the catalog. + * + * @param ident The function identifier. + * @return True if the function exists, false otherwise. + */ + default boolean functionExists(NameIdentifier ident) { + try { + getFunction(ident); + return true; + } catch (NoSuchFunctionException e) { + return false; + } + } + + /** + * 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. + */ + Function registerFunction( + NameIdentifier ident, + String comment, + FunctionType functionType, + boolean deterministic, + Type returnType, + FunctionDefinition[] definitions) + throws NoSuchSchemaException, FunctionAlreadyExistsException; + + /** + * 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. + */ + Function registerFunction( + NameIdentifier ident, + String comment, + boolean deterministic, + FunctionColumn[] returnColumns, + FunctionDefinition[] definitions) + throws NoSuchSchemaException, FunctionAlreadyExistsException; Review Comment: The javadoc states "The function definitions" but doesn't specify constraints on the array. Should document whether the definitions array can be null or empty, and what happens in those cases. Given that a function needs at least one definition to be useful, consider adding validation to require at least one definition. ########## 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"); + this.name = name; + this.dataType = Preconditions.checkNotNull(dataType, "Function column type cannot be null"); + this.comment = comment; + } + + /** + * Create a {@link FunctionColumn} instance. + * + * @param name The column name. + * @param dataType The column type. + * @param comment The optional comment of the column. + * @return A {@link FunctionColumn} instance. + */ + public static FunctionColumn of(String name, Type dataType, String comment) { + return new FunctionColumn(name, dataType, comment); + } + + /** + * @return The column name. + */ + public String name() { + return name; + } + + /** + * @return The column type. + */ + public Type dataType() { + return dataType; + } + + /** + * @return The optional column comment, null if not provided. + */ + public String comment() { + return comment; + } Review Comment: The comment parameter can be null (based on line 39 where it's assigned directly), but the method lacks a @Nullable annotation. For consistency with project practices and to help static analysis tools, add @Nullable annotation to this method's return type. ########## api/src/main/java/org/apache/gravitino/function/FunctionCatalog.java: ########## @@ -0,0 +1,147 @@ +/* + * 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 org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.annotation.Evolving; +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.rel.types.Type; + +/** The FunctionCatalog interface defines the public API for managing functions in a schema. */ +@Evolving +public interface FunctionCatalog { + + /** + * 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. + */ + NameIdentifier[] listFunctions(Namespace namespace) throws NoSuchSchemaException; + + /** + * Get a function by {@link NameIdentifier} from the catalog. The identifier only contains the + * schema and function name. A function may include multiple definitions (overloads) in the + * result. This method returns the latest version of the function. + * + * @param ident A function identifier. + * @return The latest version of the function with the given name. + * @throws NoSuchFunctionException If the function does not exist. + */ + Function getFunction(NameIdentifier ident) throws NoSuchFunctionException; + + /** + * Get a function by {@link NameIdentifier} and version from the catalog. The identifier only + * contains the schema and function name. A function may include multiple definitions (overloads) + * in the result. + * + * @param ident A function identifier. + * @param version The function version, counted from 0. Review Comment: The javadoc for the version parameter states "counted from 0", but typically version numbers in software start from 1 for the first version, not 0. Starting from 0 may be confusing to users. Consider starting versions from 1, or clearly document the rationale for 0-based versioning. If 0-based versioning is intentional, ensure this is consistently documented throughout the API, including in the Function.version() method. ```suggestion * @param version The zero-based function version index (0 for the first created version), as * returned by {@link Function#version()}. ``` ########## api/src/main/java/org/apache/gravitino/function/Function.java: ########## @@ -0,0 +1,82 @@ +/* + * 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 javax.annotation.Nullable; +import org.apache.gravitino.Auditable; +import org.apache.gravitino.annotation.Evolving; +import org.apache.gravitino.rel.types.Type; + +/** Represents a user-defined function registered in Gravitino. */ +@Evolving +public interface Function extends Auditable { + + /** + * @return The function name. + */ + String name(); + + /** + * @return The function type. + */ + FunctionType functionType(); + + /** + * @return Whether the function is deterministic. + */ + boolean deterministic(); + + /** + * @return The optional comment of the function. + */ + @Nullable + default String comment() { + return null; + } + + /** + * The return type for scalar or aggregate functions. + * + * @return The return type, null if this is a table-valued function. + */ + @Nullable + default Type returnType() { + return null; + } + + /** + * The output columns for a table-valued function. + * + * @return The output columns of a table-valued function, or an empty array for scalar or + * aggregate functions. + */ + default FunctionColumn[] returnColumns() { + return new FunctionColumn[0]; + } Review Comment: The returnColumns() method returns a new array instance on each call (as documented "an empty array"), which could lead to unnecessary object allocations in hot paths. Consider returning a shared empty array constant (similar to how FunctionResources.EMPTY is used) to avoid creating new empty arrays repeatedly. ########## api/src/main/java/org/apache/gravitino/function/FunctionDefinitions.java: ########## @@ -0,0 +1,97 @@ +/* + * 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 java.util.Arrays; + +/** Helper methods to create {@link FunctionDefinition} instances. */ +public final class FunctionDefinitions { + + private FunctionDefinitions() {} + + /** + * Create a {@link FunctionDefinition} instance. + * + * @param parameters The parameters for this definition, maybe empty but not null. + * @param impls The implementations for this definition, it must not be null or empty. + * @return A {@link FunctionDefinition} instance. + */ + public static FunctionDefinition of(FunctionParam[] parameters, FunctionImpl[] impls) { + return new FunctionDefinitionImpl(parameters, impls); + } + + private static final class FunctionDefinitionImpl implements FunctionDefinition { + private final FunctionParam[] parameters; + private final FunctionImpl[] impls; + + FunctionDefinitionImpl(FunctionParam[] parameters, FunctionImpl[] impls) { + this.parameters = parameters == null ? new FunctionParam[0] : copy(parameters); + Preconditions.checkArgument( + impls != null && impls.length > 0, "Impls cannot be null or empty"); + this.impls = copy(impls); + } + + @Override + public FunctionParam[] parameters() { + return copy(parameters); + } + + @Override + public FunctionImpl[] impls() { + return copy(impls); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (!(obj instanceof FunctionDefinitionImpl)) { + return false; + } + FunctionDefinitionImpl that = (FunctionDefinitionImpl) obj; + return Arrays.equals(parameters, that.parameters) && Arrays.equals(impls, that.impls); Review Comment: The equals method only checks for FunctionDefinitionImpl instances, which means it won't correctly compare with other implementations of the FunctionDefinition interface. This violates the Liskov Substitution Principle and could cause unexpected behavior when comparing definitions from different sources. The equals check should be against the FunctionDefinition interface type and compare using the interface methods (parameters(), impls()) to ensure compatibility with all implementations. ```suggestion if (!(obj instanceof FunctionDefinition)) { return false; } FunctionDefinition that = (FunctionDefinition) obj; return Arrays.equals(parameters, that.parameters()) && Arrays.equals(impls, that.impls()); ``` -- 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]
