[client-v2] Move SerDe related logic to a separate package #2378
Conversation
|
Maybe we also need to to provide a getter method to retrieve the POJO SerDe? |
chernser
changed the title
|
|
/windsurf-review |
There was a problem hiding this comment.
Pull Request Overview
This PR moves all serialization/deserialization (SerDe) logic from the insert and query packages into a dedicated "serde" package. Key changes include updating package declarations and renaming interfaces and methods to better reflect their SerDe role.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| SerializerNotFoundException.java | Updated package declaration from "insert" to "serde". |
| POJOSerDe.java | New file implementing class-based registration for POJO serialization/deserialization. |
| POJOFieldSerializer.java and POJOFieldDeserializer.java | Renamed interfaces to align with new SerDe terminology and moved their packages. |
| DataSerializationException.java | Updated to use the new POJOFieldSerializer type. |
| SerializerUtils.java | Updated to return POJOFieldDeserializer in compile method and update corresponding reference. |
| AbstractBinaryFormatReader.java | Replaced deprecated POJOSetter with POJOFieldDeserializer in object deserialization. |
| Client.java | Refactored code to leverage POJOSerDe for POJO registration and serializer/deserializer lookup. |
Comments suppressed due to low confidence (1)
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java:798
- The method name 'compilePOJOSetter' is inconsistent with its return type POJOFieldDeserializer; consider renaming it to 'compilePOJOFieldDeserializer' for clarity.
public static POJOFieldDeserializer compilePOJOSetter(Method setterMethod, ClickHouseColumn column) {
| import java.util.concurrent.ConcurrentHashMap; | ||
|
|
||
| /** | ||
| * This class glues serialization staff together |
There was a problem hiding this comment.
[nitpick] There appears to be a typo in the javadoc ('staff' should likely be 'stuff' or rephrased appropriately); consider revising for clarity.
| * This class glues serialization staff together | |
| * This class glues serialization components together |
Copilot uses AI. Check for mistakes.
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Other comments (6)
- client-v2/src/main/java/com/clickhouse/client/api/serde/POJOSerDe.java (62-63) The lambda in `registerClass` calls `getterMethod.invoke(obj)` without handling potential exceptions like `IllegalAccessException` or `InvocationTargetException`. This could lead to unexpected behavior or difficult-to-debug issues. Consider adding proper exception handling.
- client-v2/src/main/java/com/clickhouse/client/api/Client.java (126-126) This PR moves serialization logic to a separate package, which is a good refactoring. However, I don't see any tests added for the new `POJOSerDe` class. According to the project guidelines, tests are required for newly added functionality. Please add unit tests that verify the functionality of the new `POJOSerDe` class and its integration with the Client class.
- client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/AbstractBinaryFormatReader.java (11-11) This PR is moving serialization/deserialization logic to a separate package, but I don't see any tests included for this change. According to our guidelines, we should require tests for changes and newly added functionality to ensure the refactoring doesn't introduce regressions.
- client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java (798-798) This PR renames `POJOSetter` to `POJOFieldDeserializer` and moves it to a new package, but there are no tests included to verify that the functionality still works correctly after this change. Please add tests to ensure the renamed class functions as expected.
- client-v2/src/main/java/com/clickhouse/client/api/serde/POJOFieldDeserializer.java (13-13) This PR renames and moves the interface to a more appropriate package, which is good. However, according to our guidelines, we require tests for changes. Please add tests that verify the functionality of the `POJOFieldDeserializer` interface in its new location.
-
client-v2/src/main/java/com/clickhouse/client/api/serde/POJOSerDe.java (97-107)
The logic for determining the schema key from `TableSchema` is duplicated in `getFieldSerializers` and `getFieldDeserializers` methods. Consider extracting this to a private helper method to avoid duplication and ensure consistent behavior.
private String getSchemaKey(TableSchema schema) { return schema.getTableName() == null ? schema.getQuery() : schema.getTableName(); } public Map<String, POJOFieldSerializer> getFieldSerializers(Class<?> clazz, TableSchema schema) { // TODO: instead of storing a query list of columns should be stored and it will work both for read and write return serializers.getOrDefault(clazz, Collections.emptyMap()) .getOrDefault(getSchemaKey(schema), Collections.emptyMap()); } public Map<String, POJOFieldDeserializer> getFieldDeserializers(Class<?> clazz, TableSchema schema) { return deserializers.getOrDefault(clazz, Collections.emptyMap()).getOrDefault(getSchemaKey(schema), Collections.emptyMap()); }
|
|
||
| import java.io.IOException; | ||
| import java.io.OutputStream; | ||
| import java.lang.reflect.InvocationTargetException; | ||
|
|
||
| public interface POJOSerializer { | ||
| public interface POJOFieldSerializer { |
There was a problem hiding this comment.
This PR moves and renames the serialization interface, but I don't see any tests included to verify that existing code using this interface continues to work correctly after these changes. According to our guidelines, we should include tests for all changes. Please add tests that verify the functionality with the renamed interface.
| /** | ||
| * This class glues serialization staff together | ||
| */ | ||
| public class POJOSerDe { |
There was a problem hiding this comment.
This is a new class with complex serialization/deserialization logic, but I don't see any corresponding unit tests. Please add tests to verify the functionality of this class, especially around the method matching, serialization/deserialization, and schema key handling.
| Map<String, POJOFieldSerializer> schemaSerializers = new HashMap<>(); | ||
| Map<String, POJOFieldDeserializer> schemaDeserializers = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
There's inconsistent use of collection types - schemaSerializers uses HashMap while schemaDeserializers uses ConcurrentHashMap. Since both are used in similar contexts, they should use the same collection type for consistency and to avoid potential thread safety issues.
There was a problem hiding this comment.
Inconsistency is not a problem. Here is more question a need of concurrent collections. What should I used?
Summary
Closes
Checklist
Delete items not relevant to your PR: