[fix](streaming-job) Support delete sign in CDC stream TVF#65146
Open
JNSimba wants to merge 5 commits into
Open
[fix](streaming-job) Support delete sign in CDC stream TVF#65146JNSimba wants to merge 5 commits into
JNSimba wants to merge 5 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found one blocking regression-test issue on the refreshed head.
Critical checkpoints:
- Goal: TVF schema exposure for CDC delete sign is implemented, but the new regression coverage cannot run in normal comparison mode because expected output files are missing.
- Scope/focus: implementation is small and localized to CdcStreamTableValuedFunction plus tests.
- Concurrency/lifecycle: no new concurrency or lifecycle management introduced.
- Config/compatibility:
include_delete_signis a TVF property validated as boolean; no new FE image/edit-log field found. - Parallel paths: MySQL and PostgreSQL delete-sign paths are covered by the new suites in intent, but both need generated
.outfiles. - Tests: FE unit test was attempted but blocked before Java execution by missing
thirdparty/installed/bin/protoc;git diff --checkpassed.
Subagent conclusions:
- optimizer-rewrite: no candidates.
- tests-session-config: no candidates beyond MAIN-1 after convergence.
- C1 was invalidated by the PR head moving from 173721a to 1b4595d.
- C2 ended with both live subagents reporting
NO_NEW_VALUABLE_FINDINGSfor the current ledger/comment set.
User focus: no additional focus was provided.
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Member
Author
|
/review |
Member
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Automated review summary for PR 65146.
I did not find any additional non-duplicate inline issues in the effective four-file PR diff. The existing inline thread #65146 (comment) remains the blocking item: the two new CDC delete-sign regression suites add qt_* checks but the matching .out files are not present under regression-test/data/job_p0/streaming_job/cdc/tvf/.
Critical checkpoint conclusions:
- Goal/test proof: the FE change exposes
__DORIS_DELETE_SIGN__only wheninclude_delete_sign=true, and the new unit/regression tests target that behavior; regression expected outputs are still missing as already commented. - Scope: implementation is narrowly contained to CDC stream TVF schema exposure plus tests.
- Concurrency/lifecycle: no new shared mutable concurrency or lifecycle ownership path is introduced.
- Configuration/compatibility: the new TVF property is optional, strictly boolean-validated, defaults false, and is not persisted as new job state.
- Parallel paths: MySQL and PostgreSQL CDC TVF paths are both covered; CDC client already emits the delete-sign field on READ/CREATE/UPDATE/DELETE records.
- Test coverage/results: FE unit coverage exists, but the regression
.outfiles must be generated and committed before the suites can pass.
Subagent conclusions: optimizer-rewrite and tests-session-config subagents found no new valuable candidates beyond the existing missing-output thread. Convergence round 1 ended with both subagents replying NO_NEW_VALUABLE_FINDINGS for the same ledger and proposed final comment set.
Contributor
TPC-H: Total hot run time: 30057 ms |
Contributor
TPC-DS: Total hot run time: 175109 ms |
Contributor
ClickBench: Total hot run time: 29.17 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29836 ms |
Contributor
TPC-DS: Total hot run time: 173741 ms |
Contributor
ClickBench: Total hot run time: 25.26 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Member
Author
|
run external |
Member
Author
|
run cloud_p0 |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Member
Author
|
run external |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review result: I did not find additional blocking issues in the current head.
Critical checkpoint conclusions:
- Goal and coverage: the PR exposes
include_delete_signoncdc_stream, keeps the default schema unchanged, validates invalid values, and covers MySQL/PostgreSQL snapshot, insert, update, and delete-sign delete behavior. - Scope: the code change is focused on TVF schema exposure and targeted tests; it does not introduce persistence, edit-log, or FE-BE protocol changes.
- Data correctness: CDC client rows already emit
__DORIS_DELETE_SIGN__; the PR only exposes that field as an optional TVF column and the regression suites explicitly bind it into Unique Key MoW target tables. - Parallel paths: scheduled streaming tasks preserve the original TVF properties during
JdbcTvfSourceOffsetProvider.rewriteTvfParams(), soinclude_delete_signremains available after task-time offset rewrite. - Concurrency/lifecycle: no new shared mutable lifecycle or lock path was introduced.
- Compatibility/config: default
falsepreserves existing output schema; the new option is strict boolean validated and is intentionally removed from the HTTP payload because it is an FE schema switch. - Tests/results: FE unit coverage and MySQL/PostgreSQL regression suites with matching
.outfiles are present. The prior inline thread about missing.outfiles is stale for this head. - Validation: local FE/regression execution was not possible because
thirdparty/installedandthirdparty/installed/bin/protocare missing in this worktree. GitHub currently reports FE UT, CheckStyle, P0 regression, non-concurrent regression, and related checks passing; external regression is still pending. - User focus: no additional user-provided focus was supplied.
Subagent conclusions:
optimizer-rewrite: no candidate became an inline comment; suspicious ALTER/JDBC TVF rewrite points were dismissed with code evidence.tests-session-config: no candidate became an inline comment; the existing missing-output concern is addressed by the current.outfiles.- Final convergence round: both live subagents returned
NO_NEW_VALUABLE_FINDINGSfor the same current ledger/comment set after the proposed final comment set was empty.
Member
Author
|
run external |
Contributor
FE Regression Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
include_delete_signparameter to the CDC stream TVF; when enabled, it outputs__DORIS_DELETE_SIGN__.