[fix](Nereids) fix cancel task jobs failed #44755
Open
Conversation
This file contains 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
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morrySnow
reviewed
Nov 28, 2024
| @@ -79,14 +80,17 @@ private void validate() throws AnalysisException { | |||
| throw new AnalysisException("JobName value must is string"); | |||
| } | |||
| this.jobName = ((StringLikeLiteral) expr.child(0).child(1)).getStringValue(); | |||
| String taskIdInput = ((StringLikeLiteral) expr.child(1).child(0)).getStringValue(); | |||
| if (!(expr.child(1).child(0) instanceof UnboundSlot)) { | |||
There was a problem hiding this comment.
if jobName must string, i think we should change syntax to ensure it
There was a problem hiding this comment.
maybe u should use get text from identifier to keep it same as legacy planner
LiBinfeng-01
force-pushed
the
fix_cancel_task_job
branch
from
fd28101 to
00c6e5b
Compare
|
run buildall |
TPC-H: Total hot run time: 39831 ms |
TPC-DS: Total hot run time: 197890 ms |
ClickBench: Total hot run time: 33.14 s |
LiBinfeng-01
force-pushed
the
fix_cancel_task_job
branch
from
00c6e5b to
2816bc2
Compare
|
run buildall |
TPC-H: Total hot run time: 39869 ms |
TPC-DS: Total hot run time: 191193 ms |
ClickBench: Total hot run time: 32.95 s |
morrySnow
requested changes
Dec 3, 2024
| | DROP JOB (IF EXISTS)? wildWhere? #dropJob | ||
| | RESUME JOB wildWhere? #resumeJob | ||
| | CANCEL TASK wildWhere? #cancelJobTask | ||
| | PAUSE JOB WHERE (jobNameKey=STRING_LITERAL) EQ (jobNameValue=STRING_LITERAL) #pauseJob |
There was a problem hiding this comment.
jobNameKey is not a string literal. should be identifier
|
run buildall |
TPC-H: Total hot run time: 40235 ms |
TPC-DS: Total hot run time: 197336 ms |
ClickBench: Total hot run time: 32.78 s |
CalvinKirs
reviewed
Dec 4, 2024
| @@ -59,36 +44,9 @@ public <R, C> R accept(PlanVisitor<R, C> visitor, C context) { | |||
|
|
|||
| @Override | |||
| public void run(ConnectContext ctx, StmtExecutor executor) throws Exception { | |||
| validate(); | |||
There was a problem hiding this comment.
Did we miss the permission check?
There was a problem hiding this comment.
yes, mistoken delete
|
run buildall |
TPC-H: Total hot run time: 40163 ms |
TPC-DS: Total hot run time: 191123 ms |
ClickBench: Total hot run time: 32.34 s |
|
run buildall |
TPC-H: Total hot run time: 39688 ms |
TPC-DS: Total hot run time: 189460 ms |
ClickBench: Total hot run time: 33.37 s |
CalvinKirs
previously approved these changes
Dec 11, 2024
starocean999
previously approved these changes
Dec 11, 2024
LiBinfeng-01
dismissed stale reviews from starocean999 and CalvinKirs
via
c32dabf
|
run p0 |
morrySnow
reviewed
Dec 18, 2024
| @@ -770,41 +770,38 @@ public LogicalPlan visitCreateScheduledJob(DorisParser.CreateScheduledJobContext | |||
| return new CreateJobCommand(createJobInfo); | |||
| } | |||
|
|
|||
| private void checkJobNameKey(String key, String keyFormat) { | |||
| if (key.isEmpty() || !key.equalsIgnoreCase(keyFormat)) { | |||
| @@ -770,41 +770,38 @@ public LogicalPlan visitCreateScheduledJob(DorisParser.CreateScheduledJobContext | |||
| return new CreateJobCommand(createJobInfo); | |||
| } | |||
|
|
|||
| private void checkJobNameKey(String key, String keyFormat) { | |||
| if (key.isEmpty() || !key.equalsIgnoreCase(keyFormat)) { | |||
| throw new ParseException(keyFormat + " should be: '" + keyFormat + "'"); | |||
There was a problem hiding this comment.
error message is not very clear, we should use ParseException with Origin, or different statement use different error message to let user could know the error reason easily
Comment on lines
800
to
801
| checkJobNameKey(stripQuotes(ctx.jobNameKey.getText()), "jobName"); | ||
| checkJobNameKey(stripQuotes(ctx.taskIdKey.getText()), "taskId"); |
There was a problem hiding this comment.
i don't think must write jobName before taskId is a good idea. since we use where here, we should support jobName = xxx and taskId = xx and taskId = xx and jobName = xxx
|
run buildall |
1 similar comment
|
run buildall |
LiBinfeng-01
force-pushed
the
fix_cancel_task_job
branch
from
e33f31c to
a783a59
Compare
|
run buildall |
TPC-H: Total hot run time: 32409 ms |
TPC-DS: Total hot run time: 188210 ms |
ClickBench: Total hot run time: 30.9 s |
morrySnow
reviewed
Jan 22, 2025
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java
Outdated
Show resolved
Hide resolved
|
run buildall |
TPC-H: Total hot run time: 32375 ms |
TPC-DS: Total hot run time: 188201 ms |
ClickBench: Total hot run time: 30.13 s |
morrySnow
approved these changes
Jan 22, 2025
|
PR approved by at least one committer and no changes requested. |
github-actions
bot
added
the
approved
|
PR approved by anyone and no changes requested. |
|
run feut |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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: close #xxx
Related PR: #44279
Problem Summary:
when cancel task, it should use "taskId" as unbound slot in and right child's left child, otherwise it would failed
Regression test to be added by @CalvinKirs
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)