DanielLeens commented on PR #10306:
URL: https://github.com/apache/seatunnel/pull/10306#issuecomment-5229130380
Thanks @SEZ9 for double-checking @nzw921rx's findings. I want to flag
something important before we act on this: both blockers you just confirmed
were checked against head `2a56e4fdb` (nzw921rx's review head from 2026-07-29),
but the PR has moved well past that point since then — the current head is
`08af4675b78a6f5b9ba3ba75efbc40476bc7748f`, unchanged since my last review on
2026-08-07T07:01:24Z. I independently re-verified both items directly against
the current head, not the one you cited:
**On #1 (auto_commit=false silent data loss):** This exact class of bug is
real on `2a56e4fdb`, but it's already fixed. I traced it live across my
2026-08-03/08-06 review rounds: the original relaxation (`autoCommit &&
autoFlushed` → `autoFlushed`) that let `reportAndClearPendingRowsIfCommitted()`
clear/report rows before a real commit was replaced with a savepoint-based
boundary. On the current head, `JdbcSinkWriter.java:376-410`
(`markSuccessfulAutoFlushBoundaryIfNeeded()` / `lastSuccessfulBatchSavepoint`)
marks a savepoint instead of committing on auto-flush, and `rollbackIfNeeded()`
(`JdbcSinkWriter.java:487-506`) now rolls back **to that savepoint**, not the
whole transaction, when a later batch hits a row-level failure. So A1/A2 in
your scenario are never forgotten — they stay live in the still-open
transaction and get committed for real at `prepareCommit(checkpointId)`,
exactly aligned with the checkpoint boundary. I pulled the current file content
directly from the API
to confirm this code is present today, not just in an old review.
**On #2 (SQL query-level failures classified as row errors):** Also already
fixed, and I can show you the exact diff. At `2a56e4fdb` (the head you
reviewed), `SQLTransform.classifyRowError()` did unconditionally map every
`WHERE_STATEMENT_ERROR` to `ROW_ERROR` — I pulled that historical file content
and confirmed it. On the current head, `SQLTransform.java:122-137` now does:
```java
if (transformException.getSeaTunnelErrorCode()
== TransformCommonErrorCode.WHERE_STATEMENT_ERROR
&& !hasUnsupportedOperationCause(transformException)) {
return RowErrorClassification.ROW_ERROR;
}
return RowErrorClassification.SYSTEM_ERROR;
```
`hasUnsupportedOperationCause()` walks the cause chain for
`CommonErrorCodeDeprecated.UNSUPPORTED_OPERATION` and, if found, the classifier
now returns `SYSTEM_ERROR` instead — so an unsupported query expression like
`BETWEEN` no longer gets silently routed/dropped as a row error. There's also a
dedicated regression test, `SQLTransformTest#testClassifyRowError`, that
reproduces your exact `"id BETWEEN 1 AND 5"` example with an
`UNSUPPORTED_OPERATION`-coded cause and asserts `SYSTEM_ERROR`. This is
precisely the test nzw921rx asked for.
So: no new blocker here from my side, but a process note — both items are
already resolved on the current, unchanged head. As I posted in my full
re-review on 2026-08-07 (`08af4675b`), I found no outstanding source-level
correctness blocker on this PR after the eviction-detection fix landed; the
only remaining gates are (1) a green `Build` run on this exact head with the
three previously-unexplained connector-IT failures resolved or explained, since
the current head has never actually been through CI, and (2) the required
maintainer approval. Could you re-run your check against `08af4675b` directly
rather than the `2a56e4fdb` snapshot? I don't want us to re-litigate fixed
issues and stall the CI/approval step that's actually still open.
--
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]
