Xiao-zhen-Liu commented on code in PR #8750: URL: https://github.com/apache/texera/pull/8750#discussion_r4211086402
########## common/workflow-core/src/main/scala/org/apache/texera/amber/core/state/FixedAtCompileTime.java: ########## @@ -0,0 +1,39 @@ +/* + * 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. + */ + +package org.apache.texera.amber.core.state; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Marks a descriptor property that the compiled plan is built from: the name or type of an output Review Comment: This list is maintained by hand, and an operator added later that forgets the mark silently brings back the null column; nothing checks the list. Please state the rule in `LogicalOp`'s documentation, where operator authors will see it. ########## common/workflow-core/src/main/scala/org/apache/texera/amber/core/state/FixedAtCompileTime.java: ########## @@ -0,0 +1,39 @@ +/* + * 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. + */ + +package org.apache.texera.amber.core.state; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Marks a descriptor property that the compiled plan is built from: the name or type of an output + * column, or what an input is partitioned on. The compiler fixes the plan before any loop runs, so + * the property cannot refer to a loop variable, nor can anything inside it: the loop state would + * write the value into the operator's setting after the schema or the partitioning was already + * built from the placeholder. Inside a loop block such a reference is a compile error + * ({@code WorkflowCompiler.normalizeStateReferences}); outside every block a {@code $name} string + * stays the literal it is. + */ +@Retention(RetentionPolicy.RUNTIME) +@Target({ElementType.FIELD}) +public @interface FixedAtCompileTime { Review Comment: Nit: "compile time" reads as compiling the Scala code, not compiling the workflow before a run. What the annotation means is that the setting can't hold a loop variable; I'd name it after that, e.g. `@NoLoopVariable`. ########## amber/src/main/scala/org/apache/texera/web/service/ExecutionReconfigurationService.scala: ########## @@ -60,24 +64,48 @@ class ExecutionReconfigurationService( val newOp = modifyLogicRequest.operator val opId = newOp.operatorIdentifier val currentOp = workflow.logicalPlan.getOperator(opId) - val reconfiguredPhysicalOp = - currentOp.runtimeReconfiguration( - workflow.context.workflowId, - workflow.context.executionId, - currentOp, - newOp + if (refersToLoopVariables(currentOp) || refersToLoopVariables(newOp)) { + ModifyLogicResponse( + opId.id, + isValid = false, + s"${currentOp.operatorInfo.userFriendlyName} refers to loop variables, so it cannot be " + + "modified while the workflow runs: its executor would be rebuilt from placeholders, " + Review Comment: Nit: users won't know what "executor" or "placeholders" mean here. "Limit refers to loop variables, so it can't be changed while the workflow runs." would be enough. -- 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]
