Copilot commented on code in PR #9944: URL: https://github.com/apache/gravitino/pull/9944#discussion_r2796573221
########## clients/client-python/gravitino/dto/function/function_definition_dto.py: ########## @@ -0,0 +1,202 @@ +# 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. + +from dataclasses import dataclass, field +from typing import Any, Dict, List, Optional + +from dataclasses_json import DataClassJsonMixin, config + +from gravitino.api.function.function_column import FunctionColumn +from gravitino.api.function.function_definition import ( + FunctionDefinition, + FunctionDefinitions, +) +from gravitino.api.function.function_impl import FunctionImpl +from gravitino.api.function.function_param import FunctionParam +from gravitino.api.rel.types.json_serdes.type_serdes import TypeSerdes +from gravitino.api.rel.types.type import Type +from gravitino.dto.function.function_column_dto import FunctionColumnDTO +from gravitino.dto.function.function_impl_dto import ( + FunctionImplDTO, + JavaImplDTO, + PythonImplDTO, + SQLImplDTO, + function_impl_dto_from_function_impl, +) +from gravitino.dto.function.function_param_dto import FunctionParamDTO + + +def _decode_impl(impl_dict: Dict[str, Any]) -> FunctionImplDTO: + """Decode a function implementation DTO from a dictionary.""" + if impl_dict is None: + return None + + language = impl_dict.get("language") + if language == "SQL": + return SQLImplDTO.from_dict(impl_dict) + if language == "JAVA": + return JavaImplDTO.from_dict(impl_dict) + if language == "PYTHON": + return PythonImplDTO.from_dict(impl_dict) + + raise ValueError(f"Unsupported implementation language: {language}") + + +def _decode_impls(impls_list: List[Dict[str, Any]]) -> List[FunctionImplDTO]: + """Decode a list of function implementation DTOs from a list of dictionaries.""" + if impls_list is None: + return [] + return [_decode_impl(impl) for impl in impls_list] + + +def _encode_impl(impl: FunctionImplDTO) -> Dict[str, Any]: + """Encode a function implementation DTO to a dictionary.""" + if impl is None: + return None + result = impl.to_dict() + result["language"] = impl.language().name + return result + + +def _encode_impls(impls: List[FunctionImplDTO]) -> List[Dict[str, Any]]: + """Encode a list of function implementation DTOs to a list of dictionaries.""" + if impls is None: + return [] + return [_encode_impl(impl) for impl in impls] + + +@dataclass +class FunctionDefinitionDTO(FunctionDefinition, DataClassJsonMixin): + """DTO for function definition.""" + + _parameters: Optional[List[FunctionParamDTO]] = field( + default=None, metadata=config(field_name="parameters") + ) + _return_type: Optional[Type] = field( + default=None, + metadata=config( + field_name="returnType", + encoder=TypeSerdes.serialize, + decoder=TypeSerdes.deserialize, + ), + ) + _return_columns: Optional[List[FunctionColumnDTO]] = field( + default=None, metadata=config(field_name="returnColumns") + ) + _impls: Optional[List[FunctionImplDTO]] = field( + default=None, + metadata=config( + field_name="impls", + encoder=_encode_impls, + decoder=_decode_impls, + ), + ) + + def parameters(self) -> List[FunctionParam]: + """Returns the parameters for this definition.""" + return list(self._parameters) if self._parameters else [] + + def return_type(self) -> Optional[Type]: + """Returns the return type for scalar or aggregate functions.""" + return self._return_type + + def return_columns(self) -> List[FunctionColumn]: + """Returns the output columns for a table-valued function.""" + if self._return_columns is None: + return FunctionDefinition.EMPTY_COLUMNS + return [col.to_function_column() for col in self._return_columns] + + def impls(self) -> List[FunctionImpl]: + """Returns the implementations associated with this definition.""" + if self._impls is None: + return [] + return [impl.to_function_impl() for impl in self._impls] + + def to_function_definition(self) -> FunctionDefinition: + """Convert this DTO to a FunctionDefinition instance.""" + params = ( + [p.to_function_param() for p in self._parameters] + if self._parameters + else [] + ) + impls = [impl.to_function_impl() for impl in self._impls] if self._impls else [] + + if self._return_type is not None: + return FunctionDefinitions.of(params, self._return_type, impls) + if self._return_columns and len(self._return_columns) > 0: + cols = [col.to_function_column() for col in self._return_columns] + return FunctionDefinitions.of_table(params, cols, impls) + # Fallback for backward compatibility + return FunctionDefinitions.SimpleFunctionDefinition(params, None, None, impls) + + @classmethod + def from_function_definition( + cls, definition: FunctionDefinition + ) -> "FunctionDefinitionDTO": + """Create a FunctionDefinitionDTO from a FunctionDefinition instance.""" + param_dtos = ( + [ + ( + p + if isinstance(p, FunctionParamDTO) + else FunctionParamDTO.from_function_param(p) + ) + for p in definition.parameters() + ] + if definition.parameters() + else [] + ) + + return_column_dtos = None + if definition.return_columns() and len(definition.return_columns()) > 0: + return_column_dtos = [ + FunctionColumnDTO.from_function_column(col) + for col in definition.return_columns() + ] + + impl_dtos = ( + [function_impl_dto_from_function_impl(impl) for impl in definition.impls()] + if definition.impls() + else [] + ) + + return cls( + _parameters=param_dtos, + _return_type=definition.return_type(), + _return_columns=return_column_dtos, + _impls=impl_dtos, + ) + + def __eq__(self, other) -> bool: + if not isinstance(other, FunctionDefinitionDTO): + return False + return ( + self._parameters == other._parameters + and self._return_type == other._return_type + and self._return_columns == other._return_columns + and self._impls == other._impls + ) + + def __hash__(self) -> int: + return hash( + ( + tuple(self._parameters) if self._parameters else None, + self._return_type, + tuple(self._return_columns) if self._return_columns else None, + tuple(self._impls) if self._impls else None, Review Comment: `__hash__` currently hashes `tuple(self._impls)`, but `FunctionImplDTO` subclasses are dataclasses without a `__hash__` implementation (i.e., unhashable), so this will raise `TypeError` when `_impls` is non-empty. Consider removing `_impls` from the hash, or make the impl DTOs hashable (e.g., provide `__hash__`/`frozen=True`/`unsafe_hash=True`) in a way consistent with mutability expectations. ```suggestion len(self._impls) if self._impls is not None else None, ``` ########## clients/client-python/gravitino/dto/requests/function_update_request.py: ########## @@ -0,0 +1,231 @@ +# 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. + +from abc import ABC, abstractmethod +from dataclasses import dataclass, field +from typing import List + +from dataclasses_json import DataClassJsonMixin, config + +from gravitino.api.function.function_change import FunctionChange +from gravitino.api.function.function_impl import FunctionImpl +from gravitino.dto.function.function_definition_dto import FunctionDefinitionDTO +from gravitino.dto.function.function_impl_dto import FunctionImplDTO +from gravitino.dto.function.function_param_dto import FunctionParamDTO +from gravitino.exceptions.base import IllegalArgumentException +from gravitino.rest.rest_message import RESTRequest + + +@dataclass +class FunctionUpdateRequest(RESTRequest, ABC): + """Abstract base class for function update requests.""" + + _type: str = field(metadata=config(field_name="@type")) + + def __init__(self, action_type: str): + self._type = action_type + + @abstractmethod + def function_change(self) -> FunctionChange: + """Returns the function change.""" + pass + + +@dataclass +class UpdateCommentRequest(FunctionUpdateRequest, DataClassJsonMixin): + """Request to update the comment of a function.""" + + _new_comment: str = field(metadata=config(field_name="newComment")) + + def __init__(self, new_comment: str): + super().__init__("updateComment") + self._new_comment = new_comment + + def function_change(self) -> FunctionChange: + return FunctionChange.update_comment(self._new_comment) + + def validate(self): + pass Review Comment: `UpdateCommentRequest.validate()` is a no-op, so callers can construct an invalid update (e.g., `new_comment=None` or empty string) without any client-side validation. It should validate that `newComment` is present (and likely non-empty) and raise `IllegalArgumentException` when invalid, consistent with the other update request classes in this file. ```suggestion if self._new_comment is None or str(self._new_comment).strip() == "": raise IllegalArgumentException( "'newComment' field is required and cannot be null or empty" ) ``` ########## clients/client-python/gravitino/dto/requests/function_updates_request.py: ########## @@ -0,0 +1,48 @@ +# 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. + +from dataclasses import dataclass, field +from typing import List + +from dataclasses_json import config + +from gravitino.dto.requests.function_update_request import FunctionUpdateRequest +from gravitino.rest.rest_message import RESTRequest + + +@dataclass +class FunctionUpdatesRequest(RESTRequest): + """Represents a request with multiple function updates.""" + + _updates: List[FunctionUpdateRequest] = field(metadata=config(field_name="updates")) + + def __init__(self, updates: List[FunctionUpdateRequest]): + self._updates = updates + + def validate(self): + """Validates the request. + + Raises: + IllegalArgumentException: If the request is invalid. + """ + if self._updates: + for update in self._updates: + update.validate() Review Comment: `FunctionUpdatesRequest.validate()` silently succeeds when `_updates` is `None` or an empty list (because the loop only runs when `_updates` is truthy). This allows creating/sending a request with no updates. Please reject `None` (and likely empty) updates here, similar to other `*UpdatesRequest` validators (e.g., `CatalogUpdatesRequest` raises on null updates; `SchemaUpdatesRequest`/`FilesetUpdatesRequest` raise on empty). ```suggestion if self._updates is None: raise ValueError("Function updates must not be None.") if not self._updates: raise ValueError("Function updates must not be empty.") for update in self._updates: update.validate() ``` ########## clients/client-python/tests/unittests/test_requests.py: ########## @@ -266,3 +272,44 @@ def test_table_create_request(self): with self.assertRaisesRegex(IllegalArgumentException, exception_str): req = TableCreateRequest.from_json(json_str) req.validate() + + def test_function_register_request(self): + """Test FunctionRegisterRequest.""" + req = FunctionRegisterRequest( + name="func1", + function_type=FunctionType.SCALAR, + deterministic=True, + definitions=[], + comment="comment", + ) + json_data = json.loads(req.to_json()) + self.assertEqual("func1", json_data["name"]) + self.assertEqual("scalar", json_data["functionType"]) + self.assertTrue(json_data["deterministic"]) + self.assertEqual("comment", json_data["comment"]) Review Comment: `test_function_register_request` constructs a `FunctionRegisterRequest` with `definitions=[]`, which is invalid per `FunctionRegisterRequest.validate()` (it requires a non-empty `definitions`). This makes the test misleading and it doesn't validate a successful, well-formed request. Consider using at least one minimal `FunctionDefinitionDTO` and calling `req.validate()` for the “happy path”, and add explicit assertions for the expected validation failures (including the `returnType`/`returnColumns` rules). ########## clients/client-python/gravitino/dto/requests/function_register_request.py: ########## @@ -0,0 +1,106 @@ +# 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. + +from dataclasses import dataclass, field +from typing import List, Optional + +from dataclasses_json import config + +from gravitino.api.function.function_type import FunctionType +from gravitino.dto.function.function_definition_dto import FunctionDefinitionDTO +from gravitino.exceptions.base import IllegalArgumentException +from gravitino.rest.rest_message import RESTRequest + + +def _encode_function_type(func_type: FunctionType) -> str: + """Encode FunctionType to string.""" + if func_type is None: + return None + return func_type.value + + +def _decode_function_type(func_type_str: str) -> FunctionType: + """Decode string to FunctionType.""" + if func_type_str is None: + return None + return FunctionType.from_string(func_type_str) + + +@dataclass +class FunctionRegisterRequest(RESTRequest): + """Represents a request to register a function.""" + + _name: str = field(metadata=config(field_name="name")) + _function_type: FunctionType = field( + metadata=config( + field_name="functionType", + encoder=_encode_function_type, + decoder=_decode_function_type, + ) + ) + _deterministic: bool = field(metadata=config(field_name="deterministic")) + _definitions: List[FunctionDefinitionDTO] = field( + metadata=config(field_name="definitions") + ) + _comment: Optional[str] = field(default=None, metadata=config(field_name="comment")) + + def __init__( + self, + name: str, + function_type: FunctionType, + deterministic: bool, + definitions: List[FunctionDefinitionDTO], + comment: Optional[str] = None, + ): + self._name = name + self._function_type = function_type + self._deterministic = deterministic + self._definitions = definitions + self._comment = comment + + def validate(self): + """Validates the request. + + Raises: + IllegalArgumentException: If the request is invalid. + """ + if not self._name: + raise IllegalArgumentException( + "'name' field is required and cannot be empty" + ) + if self._function_type is None: + raise IllegalArgumentException("'functionType' field is required") + if not self._definitions: + raise IllegalArgumentException( + "'definitions' field is required and cannot be empty" + ) + + # Validate each definition has appropriate return type/columns based on function type + for definition in self._definitions: + if self._function_type == FunctionType.TABLE: + if ( + not definition.return_columns() + or len(definition.return_columns()) == 0 + ): + raise IllegalArgumentException( + "'returnColumns' is required in each definition for TABLE function type" + ) + else: + if definition.return_type() is None: + raise IllegalArgumentException( + "'returnType' is required in each definition for SCALAR or AGGREGATE function type" + ) Review Comment: `FunctionRegisterRequest.validate()` adds branching validation for TABLE vs SCALAR/AGGREGATE return schema (`returnColumns` vs `returnType`), but the unit tests added in this PR don't exercise these new paths. Please add tests that cover: (1) TABLE functions missing `returnColumns` -> validation error, and (2) SCALAR/AGGREGATE functions missing `returnType` -> validation error, plus a passing case for each. -- 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]
