mchades commented on PR #9943: URL: https://github.com/apache/gravitino/pull/9943#issuecomment-3881982062
Addressed review comments: 1. **RuntimeType.from_string() exception type** (function_impl.py:68): Fixed. Now iterates over enum values and raises `ValueError` for unsupported runtimes, consistent with `FunctionType.from_string()` and `SortDirection.from_string()` patterns in the codebase. 2. **SimpleFunctionParam.__hash__() missing default_value** (function_param.py:94): Fixed. Added `default_value` to `__hash__()` to match `__eq__()` and the Java `hashCode()` implementation. 3. **FunctionDefinitions.of() missing return_type validation** (function_definition.py:161): The Java API (`FunctionDefinitions.of()`) also does not validate `returnType` — it trusts the caller. This is by design. Will review whether the Java side should also add validation separately. 4. **FunctionDefinitions.of_table() missing return_columns validation** (function_definition.py:180): Same as above — the Java API doesn't validate `returnColumns` in the factory method either. Will review together with comment 3. 5. **Missing unit tests** (function_type.py:57): This PR defines pure API model types mirroring the Java API. Unit tests will be added when the client implementation that uses these models is built in a follow-up PR. 6. **PR description vs implementation** (function_change.py:115): PR description has been corrected to accurately reflect the implemented change types. 7. **SimpleFunctionDefinition constructor mutual exclusion** (function_definition.py:99): The Java `FunctionDefinitionImpl` constructor also does not validate mutual exclusivity of `returnType` vs `returnColumns`. The contract is enforced via factory methods (`of()` vs `of_table()`). Will review together with comments 3 and 4. -- 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]
