DanielLeens commented on PR #12251:
URL: https://github.com/apache/seatunnel/pull/12251#issuecomment-6574855679
Thanks @SEZ9. To save @goutamadwant a round trip, here is where F3-F8 stand
on the current head `ab016a9042bcb469c8221f662859cf69d1d92b0d`, from code and
doc reading only (nothing executed locally). This head is the plain `dev` merge
on top of `eb95e7ec`, so the Java sources and tests are the ones already
covered in my last full review.
- **F3 (`ROUTE_TO_TABLE`):** it still behaves like `FAIL` on the conversion
path, and that is documented as a limitation rather than implemented.
`JsonPathTransform.handleConversionError` (`JsonPathTransform.java:236`) throws
`ErrorDataTransformException` for any non-`SKIP` policy.
`docs/en/transforms/jsonpath.md:26` says `ROUTE_TO_TABLE` is not implemented
and is identical to `FAIL`, `:28-29` applies that to recognized conversion
failures, and `:261` repeats it in the error-handling section.
- **F4 (docs):** the `row_error_handle_way` section (`jsonpath.md:20-29`),
the column option text (`:83-85`) and the "Configure error data handle way"
section (`:254-270`) all now describe recognized conversion failures (numbers,
dates/times, binary and nested conversions) as well as path failures.
- **F5 (compat note):**
`docs/en/introduction/concepts/incompatible-changes.md:275-289` has a "JsonPath
Transform" entry saying that recognized conversion failures under `FAIL`
surface as `ErrorDataTransformException` with `JSONPATH_ERROR_CODE-07` instead
of the raw converter exception, and the sanitized-cause change. I only grepped
the English file in this pass; the Chinese file was checked in my earlier full
review of the same content.
- **F6 (test class):** unchanged. The cases are still in
`JsonPathErrorHandlingTest`, which injects converters by reflecting into the
private `converters` field for synthetic fault injection (`:666-668`). I
treated this as a non-blocking Low in my review, since the assertions are on
outcomes and would fail if the whitelist or precedence broke. If you prefer
folding the policy matrix into `JsonPathTransformTest#testErrorHandleWay`, that
is a reasonable optional cleanup, and the author can state the reason for
keeping the separate class.
- **F8 (cause):** still `initCause()`, at `JsonPathTransform.java:288`,
attaching a placeholder `JsonPathException("Original path-reading details
omitted")` to the legacy path-read error. I raised the placeholder nature as a
Low in my earlier review. @goutamadwant, a one-line note on why the cause
constructor is not used would close it for the record.
On CI: the `Build` check on this head is now `TIMED_OUT`
(https://github.com/apache/seatunnel/runs/124986656633), and it points to the
fork run https://github.com/goutamadwant/seatunnel/actions/runs/41352854679.
This is a status read, not a diagnosis. Please open that run and re-run only
the failed or timed-out jobs ("Re-run failed jobs"); if the same job fails
again, link it here. The branch is 0 commits behind `dev` and was mergeable at
the time I checked, so no sync is needed. My source-level conclusion is
unchanged: no non-CI blocker.
--
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]
