[feature](routineload) Add routine load target-table alter support#64878
Open
0AyanamiRei wants to merge 13 commits into
Open
[feature](routineload) Add routine load target-table alter support#648780AyanamiRei wants to merge 13 commits into
0AyanamiRei wants to merge 13 commits into
Conversation
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Allow paused single-table Routine Load jobs to switch their target table with ALTER ROUTINE LOAD FOR [db.]job ON <table>, while preserving existing progress and replaying the new table binding from edit log. ### Release note Support ALTER ROUTINE LOAD ... ON <table> to switch the target table for paused single-table routine load jobs. ### Check List (For Author) - Test: FE unit test - "/data/data3/huangruixin/include/src-master/apache-maven-3.9.9/bin/mvn -pl fe-core -am -DskipITs -Dcheckstyle.skip=true -DfailIfNoTests=false -Dtest=org.apache.doris.nereids.trees.plans.commands.AlterRoutineLoadCommandTest,org.apache.doris.persist.AlterRoutineLoadOperationLogTest,org.apache.doris.load.routineload.KafkaRoutineLoadJobTest test" - Kinesis unit tests skipped per user request - Behavior changed: Yes (new ALTER ROUTINE LOAD target-table switch behavior) - Does this need documentation: Yes (documented in /data/data3/huangruixin/docs/routine-load-alter-table-design.html)
0AyanamiRei
requested review from
924060929,
englefly,
gavinchou,
liaoxin01,
morrySnow,
mymeiyi and
starocean999
as code owners
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve? Issue Number: None Related PR: apache#64878 Problem Summary: Follow-up review found two issues in the new routine load target-table alter support. First, `AlterRoutineLoadCommand` had an import order regression that could fail FE checkstyle. Second, alter validation only rechecked `PARTIAL_COLUMNS=true` from the current command, which left the effective partial-update state under-validated when the existing job or a `unique_key_update_mode` change required merge-on-write semantics. This change restores import ordering, validates the effective unique key update mode against the destination table, and adds focused FE unit coverage for those cases. ### Release note Routine Load alter now rejects target-table or unique-key-update changes that are incompatible with partial update requirements. ### Check List (For Author) - Test: Unit Test - Behavior changed: Yes - Does this need documentation: No
Contributor
Author
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#64878 Problem Summary: The routine load target-table alter change hit FE checkstyle in CI because one validation error message exceeded the line-length limit and a unit-test import order did not match the FE custom import ordering rule. This commit makes the minimal formatting-only fixes so the branch aligns with FE style checks. ### Release note None ### Check List (For Author) - Test: No need to test (formatting-only fix requested by reviewer; no local build or test run) - Behavior changed: No - Does this need documentation: No
0AyanamiRei
changed the title
Contributor
|
/review |
### What problem does this PR solve? Issue Number: None Related PR: apache#64878 Problem Summary: The routine load target-table alter implementation had a single-use helper for constructing the validation load descriptor snapshot, and the parser branch for table-only alter did not mark the intended phase-one scope. This commit inlines the one-off snapshot construction at the validation call site and documents that the current parser branch only supports target table alteration before future support for combining target-table and property changes. ### Release note None ### Check List (For Author) - Test: No need to test (review follow-up only; no local build or test run) - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review completed for PR 64878 at head 760890d.
I found one correctness issue: validation can accept an alter that later leaves the routine-load job in partial-update mode on a non-MoW table.
Critical checkpoints:
- Goal: the PR adds single-table routine-load target-table alteration while preserving stream progress; the implementation largely wires parser, command validation, job mutation, replay, display, and tests, but the partial-update validation path is inconsistent with mutation for one property combination.
- Scope/focus: the target-table change is small and localized to routine-load command/job/persistence paths.
- Concurrency/lifecycle: mutation is still gated by the existing paused-job write lock; no new background thread or lock order was introduced.
- Configuration/session propagation: no new config item; existing routine-load session/job properties are reused.
- Compatibility/persistence: new log field is backward-readable by current code; target-table id is replayed in Kafka and Kinesis paths.
- Parallel paths: Kafka and Kinesis both update/replay targetTableId; scheduling/task planning uses the current job tableId.
- Tests: parser/command/persistence/unit coverage and a Kafka regression were added. I verified
git diff --checkandmvn -pl fe-core -DskipTests checkstyle:check. I did not run FE compile/unit/regression tests because this checkout lacksthirdparty/installed/bin/protocandthirdparty/installed. - User focus: no additional user-provided focus was present.
Subagent conclusions: optimizer-rewrite proposed OPT-001, which became inline comment MAIN-001. tests-session-config reported no distinct candidate. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same current ledger/comment set.
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. |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29129 ms |
Contributor
TPC-DS: Total hot run time: 171607 ms |
Contributor
ClickBench: Total hot run time: 24.95 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#64878 Problem Summary: ALTER ROUTINE LOAD validation handled unique_key_update_mode and legacy partial_columns with different precedence from the mutation path. An ALTER containing unique_key_update_mode=UPSERT and partial_columns=true could pass validation on a non-MoW table, then be applied as UPDATE_FIXED_COLUMNS. Flexible partial-update ALTERs on non-MoW tables were also rejected by the generic PARTIAL_COLUMNS validation before reaching the flexible partial-update validation path. This change makes generic partial-column validation apply only to fixed partial update mode and makes the mutation path ignore legacy partial_columns when an explicit unique_key_update_mode is present. The routine load alter regression test also declares its polling counters as local variables to satisfy regression framework script checks. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.nereids.trees.plans.commands.AlterRoutineLoadCommandTest,org.apache.doris.load.routineload.KafkaRoutineLoadJobTest,org.apache.doris.load.routineload.KinesisRoutineLoadJobTest - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
### What problem does this PR solve? Issue Number: close #xxx Related PR: apache#64878 Problem Summary: The PR pipeline failed in FE checkstyle because a RoutineLoadJob line added for altering unique key update mode exceeded the 120-character limit. Wrap the containsKey call without changing behavior so FE checkstyle can pass. ### Release note None ### Check List (For Author) - Test: Manual test - ./build.sh --fe -j60 - git diff --check -- fe/fe-core/src/main/java/org/apache/doris/load/routineload/RoutineLoadJob.java - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29076 ms |
Contributor
TPC-DS: Total hot run time: 172616 ms |
Contributor
ClickBench: Total hot run time: 25.31 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#64878 Problem Summary: The routine load alter regression test checked deterministic target-table switch rows with direct Groovy assertions and reused local variable names in the same suite scope. CI reported the case as invalid, and the local framework rejected the duplicate variables during Groovy compilation. This records the deterministic source and destination row checks through qt_sql output checks, uses distinct polling counter names, and keeps Kafka topics isolated so repeated local runs do not consume retained messages. ### Release note None ### Check List (For Author) - Test: Regression test - ./run-regression-test.sh --run -d load_p0/routine_load -s test_routine_load_alter -c 'jdbc:mysql://127.0.0.1:49030/?useLocalSessionState=true&allowLoadLocalInfile=true&zeroDateTimeBehavior=round' -ha 127.0.0.1:48030 -conf enableKafkaTest=true -conf kafka_port=19194 -conf externalEnvIp=127.0.0.1 - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29371 ms |
Contributor
TPC-DS: Total hot run time: 172483 ms |
Contributor
ClickBench: Total hot run time: 25.33 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
0AyanamiRei
changed the title
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Review completed for PR 64878 at head 0f1be9e.
I did not find any new substantiated issue to raise. The previously reported partial-update precedence issue is already covered by the existing inline thread and appears fixed in the current head: explicit unique_key_update_mode now removes partial_columns from the apply map, and validation only enforces the MoW check for effective UPDATE_FIXED_COLUMNS.
Critical checkpoints:
- Goal: the PR adds single-table routine-load target-table alteration while preserving stream progress. Current parser, command validation, job mutation, journal replay, show paths, and tests are aligned with that goal.
- Scope/focus: the implementation is localized to routine-load command/job/persistence paths plus targeted parser, unit, and regression coverage.
- Concurrency/lifecycle: live mutation remains gated by the existing paused-job write lock; queued stale tasks are filtered by checkTaskInJob after pause clears the task list; no new thread or lock order was introduced.
- Configuration/session propagation: no new config item was added; existing routine-load properties and Nereids task-info propagation are reused.
- Compatibility/persistence: targetTableId is Gson-annotated in AlterRoutineLoadJobOperationLog, current-code replay handles missing legacy targetTableId as 0, and both Kafka and Kinesis replay paths apply the new target table id without resetting progress.
- Parallel paths: Kafka and Kinesis live/replay paths both update targetTableId; task planning and transaction begin use the current job tableId.
- Tests: parser/command/persistence/unit coverage and an external-Kafka regression cover the target-table switch and the fixed partial-update precedence behavior. I did not run FE build/unit/regression tests because this checkout lacks thirdparty/installed and thirdparty/installed/bin/protoc. I checked added diff lines for trailing whitespace; none were introduced by this PR.
- User focus: no additional user-provided review focus was present.
Subagent conclusions: optimizer-rewrite reported NO_NEW_VALUABLE_FINDINGS in its focused pass and again in convergence round 1. tests-session-config also reported NO_NEW_VALUABLE_FINDINGS in its focused pass and convergence round 1. No subagent candidates became inline comments, and the final proposed inline comment set is empty.
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
### What problem does this PR solve? Issue Number: None Related PR: apache#64878 Problem Summary: ALTER ROUTINE LOAD previously used the ON target-table syntax and could not combine a target switch with supported job or data-source properties. Add the explicit SET TARGET TABLE syntax, preserve Doris property constraints, validate and apply target/job/source changes atomically, and keep unspecified Kafka and Kinesis default positions unchanged. ### Release note ALTER ROUTINE LOAD now uses SET TARGET TABLE = "table" and can combine a target table switch with supported PROPERTIES and FROM data-source properties. ### Check List (For Author) - Test: Regression test / Unit Test / Build - FE routine-load and parser unit tests - load_p0/routine_load/test_routine_load_alter regression test - ./build.sh --fe -j48 - Behavior changed: Yes. ALTER ROUTINE LOAD target-table syntax and composability changed as described above. - Does this need documentation: Yes (tracked by Related PR apache#64878)
github-actions
Bot
removed
the
approved
0AyanamiRei
changed the title
### What problem does this PR solve? Issue Number: None Related PR: apache#64878 Problem Summary: Routine load target-table alteration validated the target outside the mutation critical section, Kafka datetime default offsets could leak Doris-only properties to librdkafka, and SHOW could combine an old target table name with newly altered job state. Centralize ALTER mutation under the job and target metadata locks, repeat complete target validation before mutation, filter both internal Kafka default-offset properties, and read SHOW state under the job read lock. ### Release note Fix routine load target-table alteration consistency and Kafka datetime default-offset handling. ### Check List (For Author) - Test: Unit Test - `./run-fe-ut.sh --run org.apache.doris.load.routineload.KafkaRoutineLoadJobTest,org.apache.doris.load.routineload.KinesisRoutineLoadJobTest,org.apache.doris.load.routineload.RoutineLoadJobTest` - `./build.sh --fe -j48` - Behavior changed: Yes, target-table ALTER is validated atomically, Doris-only Kafka properties are filtered, and SHOW uses a consistent job snapshot - Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Related PR: apache#64878 Problem Summary: ALTER ROUTINE LOAD target-table switching is supported only for Kafka jobs. Remove the redundant Kinesis implementation and tests, restore the existing Kinesis ALTER behavior, and reject target-table changes for non-Kafka jobs during command validation. ### Release note ALTER ROUTINE LOAD SET TARGET TABLE supports Kafka Routine Load jobs only. ### Check List (For Author) - Test: Not run (per request) - Behavior changed: Yes. Kinesis jobs now explicitly reject target-table changes. - Does this need documentation: Yes. Document the Kafka-only limitation.
Contributor
Author
|
run buildall |
11 tasks
### What problem does this PR solve? Issue Number: N/A Related PR: apache#64878 Problem Summary: Restricting target-table alteration to Kafka restored the old Kinesis modifyProperties override, but RoutineLoadJob now requires subclasses to implement unprotectModifyProperties. This caused FE compilation to fail. Implement the required Kinesis hook by delegating to the existing property mutation path while keeping target-table alteration rejected for non-Kafka jobs. ### Release note None ### Check List (For Author) - Test: - Unit Test: Routine Load focused FE UT (71 tests) - FE build: ./build.sh --fe --clean -j48 - Behavior changed: No. Kinesis target-table alteration remains unsupported. - Does this need documentation: No
Contributor
Author
|
run buildall |
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
3 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.
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary:
Routine Load target-table alteration was initially limited to the target-only syntax
ALTER ROUTINE LOAD FOR [db.]job ON table. It could not alter supported job or data-source properties in the same statement.This PR replaces that syntax with the explicit, composable form:
The implementation has the following behavior and constraints:
SET TARGET TABLEcan be combined with supported job properties and Kafka/Kinesis data-source properties.COLUMNS,WHERE, andPARTITION.LOADprivilege checks, and be compatible with the existing load description, distribution mode, and partial-update settings.FROMtype must match the existing job type; Kafka and Kinesis jobs cannot be converted into each other.kafka_topic, unlike StarRocks.Release note
ALTER ROUTINE LOADnow usesSET TARGET TABLE = "table"to switch the target table and supports combining the switch with supportedPROPERTIESandFROMdata-source properties in one statement.Check List (For Author)
Test
Behavior changed:
ON tabletoSET TARGET TABLE = "table", and supported job/source properties can now be altered in the same statement.Does this need documentation?
Check List (For Reviewer who merge this PR)