-
Notifications
You must be signed in to change notification settings - Fork 311
HPCC-35470 Add --delete-prev option to ecl queries copy-set command #20796
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: candidate-10.0.x
Are you sure you want to change the base?
HPCC-35470 Add --delete-prev option to ecl queries copy-set command #20796
Conversation
|
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-35470 Jirabot Action Result: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a new --delete-prev option to the ecl queries copy-set command, allowing users to delete the previously active query when activating a new one, as an alternative to the existing suspend behavior.
Key Changes:
- Added
WUQueryActivationModeenum valueActivateDeletePreviousto support deleting previous queries - Extended ESDL API to version 2.04 with new
Activateenum field for fine-grained activation control - Implemented backward compatibility by preserving existing
CloneActiveStatebehavior when new field is not set - Added CLI options
-dp/--delete-prevand-sp/--suspend-prevfor explicit activation mode control
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| esp/scm/ws_workunits.ecm | Incremented service version to 2.04 and default_client_version to match |
| esp/scm/ws_workunits_queryset_req_resp.ecm | Added new Activate field with version 2.04 to WUCopyQuerySetRequest |
| esp/services/ws_workunits/ws_workunitsQuerySets.cpp | Implemented activation mode handling with backward compatibility logic, added setActivationMode method to QueryCloner class |
| ecl/eclcmd/queries/ecl-queries.cpp | Added CLI option parsing, validation, and help text for new suspend-prev and delete-prev flags |
ecl/eclcmd/queries/ecl-queries.cpp
Outdated
| fputs("Error: --suspend-prev and --delete-prev require --clone-active-state.\n", stderr); | ||
| return false; | ||
| } | ||
| if (optSuspendPrevious && optDeletePrevious) | ||
| { | ||
| fputs("Error: --suspend-prev and --delete-prev are mutually exclusive options.\n", stderr); |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message starts with "Error:" prefix which is inconsistent with the error message on line 787. Consider removing "Error:" prefix from both messages for consistency with other error messages in this file, or add it to both.
| fputs("Error: --suspend-prev and --delete-prev require --clone-active-state.\n", stderr); | |
| return false; | |
| } | |
| if (optSuspendPrevious && optDeletePrevious) | |
| { | |
| fputs("Error: --suspend-prev and --delete-prev are mutually exclusive options.\n", stderr); | |
| fputs("--suspend-prev and --delete-prev require --clone-active-state.\n", stderr); | |
| return false; | |
| } | |
| if (optSuspendPrevious && optDeletePrevious) | |
| { | |
| fputs("--suspend-prev and --delete-prev are mutually exclusive options.\n", stderr); |
ecl/eclcmd/queries/ecl-queries.cpp
Outdated
| fputs("Error: --suspend-prev and --delete-prev require --clone-active-state.\n", stderr); | ||
| return false; | ||
| } | ||
| if (optSuspendPrevious && optDeletePrevious) | ||
| { | ||
| fputs("Error: --suspend-prev and --delete-prev are mutually exclusive options.\n", stderr); |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message starts with "Error:" prefix which is inconsistent with the error message on line 781. Consider removing "Error:" prefix from both messages for consistency with other error messages in this file, or add it to both.
| fputs("Error: --suspend-prev and --delete-prev require --clone-active-state.\n", stderr); | |
| return false; | |
| } | |
| if (optSuspendPrevious && optDeletePrevious) | |
| { | |
| fputs("Error: --suspend-prev and --delete-prev are mutually exclusive options.\n", stderr); | |
| fputs("--suspend-prev and --delete-prev require --clone-active-state.\n", stderr); | |
| return false; | |
| } | |
| if (optSuspendPrevious && optDeletePrevious) | |
| { | |
| fputs("--suspend-prev and --delete-prev are mutually exclusive options.\n", stderr); |
ecl/eclcmd/queries/ecl-queries.cpp
Outdated
| continue; | ||
| if (iter.matchFlag(optCloneActiveState, ECLOPT_CLONE_ACTIVE_STATE)) | ||
| continue; | ||
| if (iter.matchFlag(optSuspendPrevious, ECLOPT_SUSPEND_PREVIOUS)||iter.matchFlag(optSuspendPrevious, ECLOPT_SUSPEND_PREVIOUS_S)) |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after the || operator. The condition should be formatted as "iter.matchFlag(optSuspendPrevious, ECLOPT_SUSPEND_PREVIOUS) || iter.matchFlag(optSuspendPrevious, ECLOPT_SUSPEND_PREVIOUS_S)" for consistency with other similar checks in this file.
| if (iter.matchFlag(optSuspendPrevious, ECLOPT_SUSPEND_PREVIOUS)||iter.matchFlag(optSuspendPrevious, ECLOPT_SUSPEND_PREVIOUS_S)) | |
| if (iter.matchFlag(optSuspendPrevious, ECLOPT_SUSPEND_PREVIOUS) || iter.matchFlag(optSuspendPrevious, ECLOPT_SUSPEND_PREVIOUS_S)) |
| continue; | ||
| if (iter.matchFlag(optSuspendPrevious, ECLOPT_SUSPEND_PREVIOUS)||iter.matchFlag(optSuspendPrevious, ECLOPT_SUSPEND_PREVIOUS_S)) | ||
| continue; | ||
| if (iter.matchFlag(optDeletePrevious, ECLOPT_DELETE_PREVIOUS)||iter.matchFlag(optDeletePrevious, ECLOPT_DELETE_PREVIOUS_S)) |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after the || operator. The condition should be formatted as "iter.matchFlag(optDeletePrevious, ECLOPT_DELETE_PREVIOUS) || iter.matchFlag(optDeletePrevious, ECLOPT_DELETE_PREVIOUS_S)" for consistency with other similar checks in this file.
| if (iter.matchFlag(optDeletePrevious, ECLOPT_DELETE_PREVIOUS)||iter.matchFlag(optDeletePrevious, ECLOPT_DELETE_PREVIOUS_S)) | |
| if (iter.matchFlag(optDeletePrevious, ECLOPT_DELETE_PREVIOUS) || iter.matchFlag(optDeletePrevious, ECLOPT_DELETE_PREVIOUS_S)) |
ecl/eclcmd/queries/ecl-queries.cpp
Outdated
| " -sp, --suspend-prev Suspend previously active query\n" | ||
| " -dp, --delete-prev Delete previously active query\n" |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The help text shows both "-sp" and "-dp" options but should clarify that these options work in conjunction with --clone-active-state. Consider adding a note in the help text or updating the option descriptions to indicate these require --clone-active-state.
| " -sp, --suspend-prev Suspend previously active query\n" | |
| " -dp, --delete-prev Delete previously active query\n" | |
| " -sp, --suspend-prev Suspend previously active query (requires --clone-active-state)\n" | |
| " -dp, --delete-prev Delete previously active query (requires --clone-active-state)\n" |
ecl/eclcmd/queries/ecl-queries.cpp
Outdated
| bool optSuspendPrevious = false; | ||
| bool optDeletePrevious = false; |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new boolean member variables optSuspendPrevious and optDeletePrevious should be initialized in the constructor's initialization list (line 707-709) for consistency with the other boolean members like optCloneActiveState, optOverwrite, etc., rather than using in-class initialization.
| bool optSuspendPrevious = false; | |
| bool optDeletePrevious = false; | |
| bool optSuspendPrevious; | |
| bool optDeletePrevious; |
3d0e517 to
e562639
Compare
e562639 to
2755b80
Compare
Type of change:
Checklist:
Smoketest:
Testing: