DanielCarter-stack commented on PR #10390:
URL: https://github.com/apache/seatunnel/pull/10390#issuecomment-3794745706
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10390", "part": 1,
"total": 1} -->
### Issue 1: Test coverage is not comprehensive enough
**Location**:
`seatunnel-transforms-v2/src/test/java/org/apache/seatunnel/transform/JsonPathTransformTest.java:549-662`
**Related Context**:
- Test class: `JsonPathTransformTest.java`
- Production class: `JsonPathTransform.java:130-165`
- Dependency class: `JsonToRowConverters.java:259-293`
**Problem Description**:
The newly added test cases cover the scenarios of "multiple date columns
with different formats" and "containing null values", but are missing the
following scenarios:
1. Regression test for a single date column (to verify the modification does
not break original behavior)
2. Test for multiple date columns with the same format (to verify caching
mechanism)
3. Mixed type field test (to verify it does not affect string, integer, and
other types)
**Potential Risks**:
- Risk 1: Unable to ensure compatibility of the modification with single
date column scenarios
- Risk 2: Unable to verify whether performance optimization for multiple
fields with the same format has regressed
- Risk 3: Unable to verify the modification does not affect non-date type
fields
**Impact Scope**:
- Direct impact: Test coverage of `JsonPathTransform`
- Indirect impact: Safety net for future refactoring
- Impact surface: Single Transform (not framework-level)
**Severity**: MINOR
**Improvement Suggestions**:
```java
@Test
public void testSingleDateColumn() {
// Verify that the behavior of a single date column remains consistent
with the modifications
Map<String, Object> configMap = new HashMap<>();
configMap.put(
JsonPathTransformConfig.COLUMNS.key(),
Arrays.asList(
ImmutableMap.of(
JsonPathTransformConfig.SRC_FIELD.key(), "data",
JsonPathTransformConfig.PATH.key(), "$.birth",
JsonPathTransformConfig.DEST_FIELD.key(), "birth_date",
JsonPathTransformConfig.DEST_TYPE.key(), "date")));
// ... test logic
}
@Test
public void testMultipleDateColumnsWithSameFormat() {
// Verify that multiple date columns with the same format can share cache
Map<String, Object> configMap = new HashMap<>();
configMap.put(
JsonPathTransformConfig.COLUMNS.key(),
Arrays.asList(
ImmutableMap.of(..., "birth", "birth_date", "date"),
ImmutableMap.of(..., "hired", "hire_date", "date")));
String jsonData = "{\"birth\": \"2024-01-15\", \"hired\":
\"2024-02-20\"}";
// ... verify both fields are correctly parsed
}
@Test
public void testMixedTypeColumns() {
// Verify mixed configuration of date columns with string and integer
columns
Map<String, Object> configMap = new HashMap<>();
configMap.put(
JsonPathTransformConfig.COLUMNS.key(),
Arrays.asList(
ImmutableMap.of(..., "date_field", "date"),
ImmutableMap.of(..., "string_field", "string"),
ImmutableMap.of(..., "int_field", "int")));
// ... verify all types are correctly converted
}
```
**Rationale**:
- These tests can enhance confidence in the correctness of the modification
- Single date column test ensures backward compatibility
- Same format test verifies cache sharing works properly
- Mixed type test verifies isolation of the modification (does not affect
other types)
---
### Issue 2: Missing CHANGELOG update
**Location**: `CHANGELOG.md` (root directory)
**Related Context**:
- Modified file: `JsonPathTransform.java`
- Modification type: Bug Fix
- PR number: #10390
**Problem Description**:
The PR fixes a user-visible bug (parsing failure for multiple date columns),
but the description does not mention updating `CHANGELOG.md`.
**Potential Risks**:
- Risk 1: Users cannot perceive this bug fix from the CHANGELOG
- Risk 2: Incomplete change record when version is released
**Impact Scope**:
- Direct impact: `CHANGELOG.md` documentation
- Indirect impact: Channel for users to learn about bug fixes
- Impact surface: Documentation (non-functional)
**Severity**: MINOR
**Improvement Suggestions**:
Add to `CHANGELOG.md`:
```markdown
## 2.3.6 (Unreleased)
### Bug Fixes
* [Transform-V2] Fix JsonPath transform date/time conversion error when
multiple date columns exist. Previously, when using JsonPath transform with
multiple date/timestamp columns, the converter received `null` as the field
name, causing incorrect date format resolution. Now each column correctly
applies its configured date/time format (#10390)
```
**Rationale**:
- CHANGELOG is the primary channel for users to learn about version changes
- This bug fix is important for affected users
- Recording in CHANGELOG helps users decide whether to upgrade
---
### Issue 4: Insufficient encapsulability of fieldFormatterMap
**Location**:
`seatunnel-formats/seatunnel-format-json/src/main/java/org/apache/seatunnel/format/json/JsonToRowConverters.java:80`
**Related Context**:
- Current definition: `public Map<String, DateTimeFormatter>
fieldFormatterMap = new HashMap<>();`
- Usage location: `JsonToRowConverters.java:261, 281`
- Caller: `JsonPathTransform.java` (indirectly through converter)
**Problem Description**:
`fieldFormatterMap` is declared as `public`, which is an unnecessary
encapsulation violation. Although it does need to be accessed externally (e.g.,
by tests), a better approach is to provide a controlled access method.
**Potential Risks**:
- Risk 1: External code may directly modify the Map, causing cache pollution
- Risk 2: Difficult to change internal implementation during future
refactoring (e.g., replacing with ConcurrentMap)
**Impact Scope**:
- Direct impact: API design of `JsonToRowConverters`
- Indirect impact: All code using `JsonToRowConverters`
- Impact surface: Format layer (multiple Formats use similar patterns)
**Severity**: MINOR
**Improvement Suggestions**:
```java
// Current (line 80)
public Map<String, DateTimeFormatter> fieldFormatterMap = new HashMap<>();
// Suggested change to
private Map<String, DateTimeFormatter> fieldFormatterMap = new HashMap<>();
// If tests need access, add package-level visible access methods
Map<String, DateTimeFormatter> getFieldFormatterMap() {
return fieldFormatterMap;
}
// Or provide controlled access methods
public DateTimeFormatter getCachedFormatter(String fieldName) {
return fieldFormatterMap.get(fieldName);
}
```
**Rationale**:
- Follows encapsulation principle: internal implementation details should
not be exposed as public
- Prevents accidental modification: external code should not directly
manipulate cache
- Preserves refactoring freedom: underlying implementation can be replaced
in the future without affecting API
**Note**: This issue is outside the scope of this PR and is an existing
problem in the codebase. However, since it was discovered during review, it
should be recorded for future improvement.
---
--
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]
