-
Notifications
You must be signed in to change notification settings - Fork 714
[#9529] feat(server): Add server-side REST interface for UDFs (part-1) #9566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
bf67ede to
0b4f14c
Compare
00f0661 to
50f2504
Compare
…sponse classes Add the DTO (Data Transfer Object) layer for Function support in the REST API. This includes: **Function DTOs:** - `FunctionDTO`: Main DTO representing a function - `FunctionDefinitionDTO`: DTO for function definition with parameters and return type - `FunctionParamDTO`: DTO for function parameters - `FunctionColumnDTO`: DTO for function return columns (for table functions) - `FunctionImplDTO`: Base DTO for function implementation - `SQLImplDTO`, `JavaImplDTO`, `PythonImplDTO`: Implementation-specific DTOs - `FunctionResourcesDTO`: DTO for external resources **REST API Request/Response classes:** - `FunctionRegisterRequest`: Request for registering a new function - `FunctionUpdateRequest`: Request for updating function properties - `FunctionUpdatesRequest`: Request for batch function updates - `FunctionResponse`: Response containing a single function - `FunctionListResponse`: Response containing a list of functions Fix: apache#9561 These DTOs and request/response classes are essential for exposing Function management capabilities through the REST API. They enable serialization/deserialization between the API layer and the Function domain model. Yes. This adds the foundation for Function REST API endpoints. Added unit tests in `TestFunctionDTO` covering: - Serialization/deserialization of FunctionDTO and nested DTOs - Builder pattern usage - JSON format validation
|
@jerryshao could you plz help to review this PR when you have time? thx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements the server-side REST API infrastructure for User-Defined Functions (UDFs) in Apache Gravitino. The implementation includes comprehensive DTO classes for representing functions, their definitions, parameters, implementations (Java, Python, SQL), and resources, along with Request/Response classes for function registration and updates.
Changes:
- Added complete DTO hierarchy for UDF representation including FunctionDTO, FunctionDefinitionDTO, FunctionParamDTO, FunctionColumnDTO, and implementation classes (JavaImplDTO, PythonImplDTO, SQLImplDTO)
- Implemented REST API request/response classes for function registration (FunctionRegisterRequest), updates (FunctionUpdateRequest, FunctionUpdatesRequest), and responses (FunctionResponse, FunctionListResponse)
- Extended DTOConverters with Function to FunctionDTO conversion support
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| TestFunctionDTO.java | Comprehensive serialization/deserialization tests for all DTO classes including scalar and table functions |
| DTOConverters.java | Added toDTO method for converting Function entities to FunctionDTO with proper handling of return columns and definitions |
| FunctionResponse.java | Response wrapper for single function operations with validation for required fields |
| FunctionListResponse.java | Response wrapper for listing multiple functions with null validation |
| FunctionUpdatesRequest.java | Request class for batch function updates with update list validation |
| FunctionUpdateRequest.java | Interface and implementations for various function update operations (comment, definition, implementation changes) |
| FunctionRegisterRequest.java | Request class for function registration with validation for function type-specific requirements |
| SQLImplDTO.java | DTO for SQL-based function implementations with runtime, resources, and SQL expression |
| PythonImplDTO.java | DTO for Python function implementations with handler and code block support |
| JavaImplDTO.java | DTO for Java function implementations with class name specification |
| FunctionResourcesDTO.java | DTO for function resources (jars, files, archives) with builder pattern |
| FunctionParamDTO.java | DTO for function parameters with support for default values and type information |
| FunctionImplDTO.java | Abstract base DTO for function implementations with polymorphic deserialization support |
| FunctionDefinitionDTO.java | DTO for function definitions pairing parameters with implementations |
| FunctionDTO.java | Main DTO representing complete function metadata implementing Function interface |
| FunctionColumnDTO.java | DTO for table function return columns with name, type, and comment |
common/src/main/java/org/apache/gravitino/dto/requests/FunctionUpdateRequest.java
Outdated
Show resolved
Hide resolved
common/src/main/java/org/apache/gravitino/dto/function/FunctionDefinitionDTO.java
Show resolved
Hide resolved
common/src/main/java/org/apache/gravitino/dto/requests/FunctionRegisterRequest.java
Outdated
Show resolved
Hide resolved
common/src/test/java/org/apache/gravitino/dto/function/TestFunctionDTO.java
Show resolved
Hide resolved
| } | ||
|
|
||
| /** Builder for {@link FunctionColumnDTO}. */ | ||
| public static class Builder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use Lombok's builder annotation to auto-generate the builder class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adopted
| } | ||
|
|
||
| /** Builder for {@link FunctionParamDTO}. */ | ||
| public static class Builder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this Builder class here and above? Since we already have fromXXX help method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, fromXXX also wrap the the default value converter logic
| Assertions.assertEquals("input", deserialized.parameters()[0].name()); | ||
| Assertions.assertEquals(1, deserialized.impls().length); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also add tests for request and response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
| .withName("table_func") | ||
| .withFunctionType(FunctionType.TABLE) | ||
| .withDeterministic(false) | ||
| .withReturnColumns(new org.apache.gravitino.dto.function.FunctionColumnDTO[] {col}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LLM tends to use full qualified class name, you should avoid this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed. I opened a new issue to track this #9789
What changes were proposed in this pull request?
This PR implements the server-side REST API for User-Defined Functions (UDFs), including:
FunctionDTO,FunctionDefinitionDTO,FunctionParamDTO,FunctionColumnDTO,FunctionImplDTO(withJavaImplDTO,PythonImplDTO,SQLImplDTOsubclasses),FunctionResourcesDTOFunctionRegisterRequest,FunctionUpdateRequest,FunctionUpdatesRequest,FunctionResponseWhy are the changes needed?
To provide server-side REST API support for managing UDFs in Gravitino.
Fix: #9529
Does this PR introduce any user-facing change?
No
How was this patch tested?
TestFunctionDTO)