[Fix][Connector-clickhouse] Fix SeaTunnelRow tableId set error #9585
Conversation
Hisoka-X
changed the title
There was a problem hiding this comment.
I found our test case not check SeaTunnelRow values and tableId which return by batchFetchRecords. Please add it.
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes an issue with tableId setting in SeaTunnelRow objects for the ClickHouse connector. The main problem was that the SQL string was being incorrectly used as the tableId instead of the proper table path identifier.
- Updates the
batchFetchRecordsmethod signature to accept aTablePathparameter - Modifies all calls to
batchFetchRecordsto pass the table path for proper tableId setting - Adds comprehensive test coverage to verify tableId is correctly set in returned SeaTunnelRow objects
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ClickhouseProxy.java | Updates batchFetchRecords method signature to accept TablePath parameter and use it for tableId instead of SQL string |
| ClickhouseValueReader.java | Updates calls to batchFetchRecords to pass the table path parameter |
| ClickhouseValueReaderTest.java | Updates test mocks and adds new test to verify correct tableId setting behavior |
| ClickhouseSourceConfig.java | Adds server timezone configuration to the builder |
| public void testBatchFetchRecordsAndTableId() throws Exception { | ||
| // mock proxy query response | ||
| ClickhouseProxy proxy = Mockito.spy(new ClickhouseProxy(node)); | ||
| Field requestField = ClickhouseProxy.class.getDeclaredField("clickhouseRequest"); |
There was a problem hiding this comment.
Using reflection to access private fields makes the test fragile and tightly coupled to implementation details. Consider adding a package-private constructor or setter method to ClickhouseProxy for testing purposes instead of using reflection.
| // mock proxy query response | ||
| ClickhouseProxy proxy = Mockito.spy(new ClickhouseProxy(node)); | ||
| Field requestField = ClickhouseProxy.class.getDeclaredField("clickhouseRequest"); | ||
| requestField.setAccessible(true); |
There was a problem hiding this comment.
Setting field accessibility bypasses encapsulation and can cause issues in environments with security managers. This approach is brittle and should be replaced with proper dependency injection or test-friendly constructors.
3 participants
Purpose of this pull request
fix #9584
Does this PR introduce any user-facing change?
How was this patch tested?
Check list
New License Guide