Xiao-zhen-Liu commented on code in PR #8640: URL: https://github.com/apache/texera/pull/8640#discussion_r4212981118
########## frontend/src/app/workspace/util/loop-variable.util.ts: ########## @@ -0,0 +1,343 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { WorkflowGraphReadonly } from "../service/workflow-graph/model/workflow-graph"; + +/** + * A plain assignment target: an optional star, an identifier and an optional annotation (`x: int`). + * Identifiers are ASCII because a `$name` reference can only spell ASCII names. + */ +const ASSIGNMENT_TARGET = /^\*?\s*([A-Za-z_][A-Za-z0-9_]*)\s*(?::[^=]*)?$/; + +/** The characters that, right before `=`, make it part of another operator (`==`, `+=`, `<=`, `:=`, ...). */ +const NOT_AN_ASSIGNMENT_BEFORE = "=!<>+-*/%&|^:@"; + +/** Python's keywords, which are never a name (`else: y = 1` must not yield `else`). */ +const PYTHON_KEYWORDS: ReadonlySet<string> = new Set([ + "False", + "None", + "True", + "and", + "as", + "assert", + "async", + "await", + "break", + "class", + "continue", + "def", + "del", + "elif", + "else", + "except", + "finally", + "for", + "from", + "global", + "if", + "import", + "in", + "is", + "lambda", + "nonlocal", + "not", + "or", + "pass", + "raise", + "return", + "try", + "while", + "with", + "yield", +]); + +/** The head of a statement that opens a def or class body, whose names are local to it. */ +const LOCAL_SCOPE_HEADER = /^(?:async\s+)?(?:def|class)\b/; + +/** The head of a compound statement whose body runs at module level; group 1 is the keyword. */ +const COMPOUND_HEADER = /^(?:async\s+)?(if|elif|else|while|for|with|try|except|finally)\b/; + +const OPENING = "([{"; +const CLOSING = ")]}"; + +interface Statement { + text: string; + /** Whether the statement begins a line (as opposed to following a `;`), so that indentation is telling. */ + startsLine: boolean; +} + +/** + * The names a Loop Start's "initialization" Python code binds at module level, in first-binding order and + * without duplicates: the loop's variables, which the operators of its block may refer to as `$name`. + * The backend runs the code with exec and keeps the resulting globals as the loop state, and Python has + * no block scope, so these are the assignments at any depth outside a def or class body, whether at the + * top level or under an if/elif/else, try/except/finally, while, for or with, plus the targets of a for + * loop. + * + * Assignments: `i = 0`, `K = 2; prev = float('-inf')`, tuple targets (`a, b = 1, 2`), chained + * (`a = b = 0`), annotated (`x: int = 0`) and starred (`a, *rest = xs`) ones. Augmented assignments, + * comparisons, keyword arguments, attribute or subscript targets and comments are not assignments of a + * loop variable and are ignored, as are the names an import, a def, a class, `with ... as` or Review Comment: A Loop Start whose initialization contains `from math import pi` gives the loop a variable `pi`, but the panel doesn't know it: typing `$pi` into a setting shows "$pi is not a variable of an enclosing block" and disables Run, although the workflow would run. Names created by a `from ... import` line need to count. ########## frontend/src/app/workspace/service/validation/validation-workflow.service.ts: ########## @@ -280,15 +319,39 @@ export class ValidationWorkflowService { throw new Error("operatorSchema doesn't exist"); } - const isValid = this.ajv.validate(operatorSchema.jsonSchema, operator.operatorProperties); - if (isValid) { - return { isValid: true }; + const properties = operator.operatorProperties; + const isValid = this.ajv.validate(operatorSchema.jsonSchema, properties); + let errors = isValid ? [] : this.ajv.errors ?? []; + const validationError: Record<string, string> = {}; + + // Inside a control block a primitive property may hold a loop-variable reference ("$K") that the + // backend binds at run time (issue #8635), so the schema type check is waived exactly where a + // reference sits in a property whose schema type is a primitive (the fields the property panel lets + // take one), and every reference must name a variable an enclosing Loop Start declares. Outside every + // block nothing is waived: a "$K" in an integer property stays a type error, and so does one in an + // array or object property anywhere. + const namesInScope = loopVariablesInScope(this.workflowActionService.getTexeraGraph(), operatorID); + if (namesInScope !== undefined) { + errors = errors.filter( + error => + !( + error.keyword === "type" && + primitiveSchemaType(error.params["type"]) !== undefined && + isReference(valueAtPointer(properties, error.instancePath)) + ) + ); + const validateReference = referenceValidator(namesInScope); + const undeclared = collectReferences(properties) + .map(reference => validateReference(`$${reference.name}`)) + .find(message => message !== undefined); + if (undeclared !== undefined) { + validationError["loopVariable"] = undeclared; Review Comment: An unknown variable name disables Run. But the panel finds the loop's variables by reading the Loop Start's Python code with its own rules, which miss some names (the import above), so a workflow that would run can be blocked. A name that is really unknown already stops the run with a clear message ("property /limit refers to loop variable n, but no state message carried it"), so here it should be a warning, not a reason to disable Run. ########## frontend/src/app/workspace/service/validation/validation-workflow.service.ts: ########## @@ -227,6 +251,21 @@ export class ValidationWorkflowService { this.updateValidationState(value.operator.operatorID, this.validateOperator(value.operator.operatorID)) ); + // An operator holding a loop-variable reference is judged against the block around it and the + // variables its Loop Starts declare, which other operators' links and properties decide: a link two + // hops away can close or open its block, and renaming a Loop Start's variable can orphan "$K". So + // after every link added or removed, and every property change of a Loop Start, re-validate each Review Comment: Nit: this check and the property panel each keep their own list of the edits after which they look again at whether an operator is inside a loop (adding a link, removing a link, editing a Loop Start). If a new kind of edit is added to one list and not the other, the two disagree. One shared list would avoid that. ########## frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts: ########## @@ -960,6 +1017,25 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On mappedField.type = customType; } + // Inside a control block a primitive property may hold a loop-variable reference ("$K"), which the + // backend binds at run time (issue #8635): render it as a text input that takes "$K" next to plain + // values, offer the variables the enclosing Loop Starts declare, and flag a name none declares. + // Enums and custom widgets keep their controls; `mappedField.type` still equal to the schema type + // is what tells a plain primitive from a field formly or a widget already resolved otherwise. A + // field with value rules is a primitive too, on the rules' control (setValueRules above): the text + // input replaces that control, keeps its rules for every value but a reference, and offers the + // values they accept next to the variables. + const loopSchemaType = primitiveSchemaType(mapSource.type); + if ( + loopVariableNames !== undefined && + customType === undefined && + loopSchemaType !== undefined && + (mappedField.type === loopSchemaType || isDefined(mapSource.valueRules)) && + !isDefined(mapSource.enum) + ) { + applyLoopVariableField(mappedField, loopSchemaType, loopVariableNames); Review Comment: Inside a loop, every setting offers `$i`, including the ones #8750 forbids, such as a Projection's new column name. A user who picks `$i` there gets an error when the workflow is compiled. The panel should not offer `$i` in those settings, which needs #8750's list of them to reach the frontend. -- 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]
