kunwp1 commented on code in PR #8539:
URL: https://github.com/apache/texera/pull/8539#discussion_r4043121918
##########
frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.ts:
##########
Review Comment:
Might not be related to this PR but I want you to fix this to
`this.workflowActionService.getWorkflowMetadata()?.name`. Otherwise it returns
"getWorkflowMetadata", not the workflow name.
##########
frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.ts:
##########
Review Comment:
Maybe add a guard here to check if `this.operatorId` is set. Then you can
simplify line 482.
##########
frontend/src/app/workspace/component/result-exportation/result-exportation.component.ts:
##########
Review Comment:
Can you refactor this code to use `.getAllOperatorIDs()`; ?
##########
frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.ts:
##########
Review Comment:
Not related to this PR again but I think it's a simple fix. I want you to
add a guard at the button in the .html file
`*ngIf="guiConfigService.env.exportExecutionResultEnabled"` so that this button
is not rendered if the result export feature is disabled?
##########
frontend/src/app/workspace/component/result-exportation/result-exportation.component.ts:
##########
@@ -81,9 +81,9 @@ import { NzIconDirective } from "ng-zorro-antd/icon";
],
})
export class ResultExportationComponent implements OnInit {
- /* Two sources can trigger this dialog, one from context-menu
- which only export highlighted operators
- and second is menu which wants to export all operators
+ /* Three sources can trigger this dialog: the context-menu, which exports
the highlighted
+ operators; the menu, which wants to export all of them; and a result cell,
which names the
+ one operator whose results it shows in operatorIds below and sends no
trigger of its own.
*/
sourceTriggered: string = inject(NZ_MODAL_DATA).sourceTriggered;
Review Comment:
It's not related to this PR but can you add `?? ""` because
`sourceTriggered` is `undefined` in the download cell path.
##########
frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts:
##########
@@ -198,10 +198,14 @@ export class WorkflowResultExportService {
columnIndex: number,
filename: string,
exportAll: boolean = false, // if the user click export button on the top
bar (a.k.a menu),
- // we should export all operators, otherwise, only highlighted ones
- // which means export button is selected from context-menu
+ // we should export all operators, otherwise the ones requestedOperatorIds
names below, or
+ // the highlighted ones when it names none, which means the export button
came from the
+ // context-menu
destination: "dataset" | "local" = "dataset", // default to dataset
Review Comment:
It's not related to this PR but can you remove this default parameter value.
It's unreachable.
##########
frontend/src/app/workspace/component/result-exportation/result-exportation.component.ts:
##########
@@ -263,7 +275,8 @@ export class ResultExportationComponent implements OnInit {
this.inputFileName,
this.sourceTriggered === "menu",
destination,
- this.selectedComputingUnit
+ this.selectedComputingUnit,
+ this.operatorIds
Review Comment:
Pass `this.getOperatorIdsToCheck()` instead.
##########
frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts:
##########
@@ -198,10 +198,14 @@ export class WorkflowResultExportService {
columnIndex: number,
filename: string,
exportAll: boolean = false, // if the user click export button on the top
bar (a.k.a menu),
- // we should export all operators, otherwise, only highlighted ones
- // which means export button is selected from context-menu
+ // we should export all operators, otherwise the ones requestedOperatorIds
names below, or
+ // the highlighted ones when it names none, which means the export button
came from the
+ // context-menu
destination: "dataset" | "local" = "dataset", // default to dataset
- unit: DashboardWorkflowComputingUnit | null // computing unit for cluster
setting
+ unit: DashboardWorkflowComputingUnit | null, // computing unit for cluster
setting
+ // The operators to export, for a caller that already knows them. Ignored
when exportAll
+ // asks for everything; left out, the scope falls back to the canvas
selection.
+ requestedOperatorIds: readonly string[] = []
Review Comment:
I think the name is too long. change it to `operatorIds` and also remove the
default value.
##########
frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts:
##########
@@ -198,10 +198,14 @@ export class WorkflowResultExportService {
columnIndex: number,
filename: string,
exportAll: boolean = false, // if the user click export button on the top
bar (a.k.a menu),
Review Comment:
We can remove this `exportAll` after the change above.
##########
frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts:
##########
@@ -262,13 +268,21 @@ export class WorkflowResultExportService {
return;
}
- // Determines operator scope
+ // Determines operator scope. "Everything" wins first: the top menu means
the whole workflow
+ // whatever else it was handed. Otherwise a caller that names its
operators wins over the
+ // canvas selection, which answers a different question -- what the user
has selected. That
+ // is the context menu's scope, but not a result cell's: a cell belongs to
one operator,
+ // whoever is selected. The Form View selects the step the user is
configuring, and until
+ // they click one it selects nothing, so a cell's export there came out
with an empty scope
+ // and returned below without sending a request -- a button that did
nothing at all.
const operatorIds = exportAll
? this.workflowActionService
.getTexeraGraph()
.getAllOperators()
.map(operator => operator.operatorID)
- :
[...this.workflowActionService.getJointGraphWrapper().getCurrentHighlightedOperatorIDs()];
+ : requestedOperatorIds.length > 0
+ ? [...requestedOperatorIds]
+ :
[...this.workflowActionService.getJointGraphWrapper().getCurrentHighlightedOperatorIDs()];
Review Comment:
This code can be simplified since we pass `operatorIds`
--
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]
