Copilot commented on code in PR #4712: URL: https://github.com/apache/texera/pull/4712#discussion_r3177402429
########## common/workflow-core/src/test/scala/org/apache/texera/amber/util/VirtualIdentityUtilsSpec.scala: ########## @@ -0,0 +1,133 @@ +/* + * 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.util + +import org.apache.texera.amber.core.virtualidentity.{ + ActorVirtualIdentity, + OperatorIdentity, + PhysicalOpIdentity, + WorkflowIdentity +} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +class VirtualIdentityUtilsSpec extends AnyFlatSpec with Matchers { + + // ----- createWorkerIdentity ----- + + "createWorkerIdentity (raw fields)" should "format Worker:WF<id>-<op>-<layer>-<workerIdx>" in { + val actor = VirtualIdentityUtils.createWorkerIdentity( + WorkflowIdentity(7), + operator = "myOp", + layerName = "main", + workerId = 3 + ) + actor.name shouldBe "Worker:WF7-myOp-main-3" + } + + "createWorkerIdentity (PhysicalOpIdentity overload)" should "delegate to the same encoded format" in { + val physicalOpId = PhysicalOpIdentity(OperatorIdentity("myOp"), "main") + val actor = VirtualIdentityUtils.createWorkerIdentity( + WorkflowIdentity(7), + physicalOpId, + workerId = 3 + ) + actor.name shouldBe "Worker:WF7-myOp-main-3" + } + + // ----- getPhysicalOpId ----- + + "getPhysicalOpId" should "extract operator id and layer name from a worker actor name" in { + val actor = ActorVirtualIdentity("Worker:WF7-myOp-main-3") + val opId = VirtualIdentityUtils.getPhysicalOpId(actor) + opId.logicalOpId.id shouldBe "myOp" + opId.layerName shouldBe "main" + } + + it should "fall back to __DummyOperator/__DummyLayer for non-worker actor names" in { + val controller = ActorVirtualIdentity("CONTROLLER") + val opId = VirtualIdentityUtils.getPhysicalOpId(controller) + opId.logicalOpId.id shouldBe "__DummyOperator" + opId.layerName shouldBe "__DummyLayer" + } + + it should "tolerate operator names that contain hyphens by greedy backtracking" in { + // The operator capture group is `.+` which backtracks to leave the trailing + // `-(\w+)-(\d+)` slots populated. A multi-hyphen operator name must still + // round-trip without losing characters from the operator itself. + val actor = ActorVirtualIdentity("Worker:WF1-multi-part-op-main-0") + val opId = VirtualIdentityUtils.getPhysicalOpId(actor) + opId.logicalOpId.id shouldBe "multi-part-op" + opId.layerName shouldBe "main" + } + + // ----- getWorkerIndex ----- + + "getWorkerIndex" should "return the trailing numeric workerId from a worker actor name" in { + val actor = ActorVirtualIdentity("Worker:WF7-myOp-main-42") + VirtualIdentityUtils.getWorkerIndex(actor) shouldBe 42 + } + + // Intentionally not covered: actor names that do not match workerNamePattern + // make getWorkerIndex throw scala.MatchError because the method has no + // fallback case. See the "Potential bug" note in the PR description. Review Comment: This comment points readers to the PR description for the important context, but that context disappears once the change is merged and someone is reading the test in the source tree. Please make the note self-contained (or reference the issue number) so the reason for skipping this case stays discoverable. ########## common/workflow-core/src/test/scala/org/apache/texera/amber/util/VirtualIdentityUtilsSpec.scala: ########## @@ -0,0 +1,133 @@ +/* + * 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.util + +import org.apache.texera.amber.core.virtualidentity.{ + ActorVirtualIdentity, + OperatorIdentity, + PhysicalOpIdentity, + WorkflowIdentity +} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +class VirtualIdentityUtilsSpec extends AnyFlatSpec with Matchers { + + // ----- createWorkerIdentity ----- + + "createWorkerIdentity (raw fields)" should "format Worker:WF<id>-<op>-<layer>-<workerIdx>" in { + val actor = VirtualIdentityUtils.createWorkerIdentity( + WorkflowIdentity(7), + operator = "myOp", + layerName = "main", + workerId = 3 + ) + actor.name shouldBe "Worker:WF7-myOp-main-3" + } + + "createWorkerIdentity (PhysicalOpIdentity overload)" should "delegate to the same encoded format" in { + val physicalOpId = PhysicalOpIdentity(OperatorIdentity("myOp"), "main") + val actor = VirtualIdentityUtils.createWorkerIdentity( + WorkflowIdentity(7), + physicalOpId, + workerId = 3 + ) + actor.name shouldBe "Worker:WF7-myOp-main-3" + } + + // ----- getPhysicalOpId ----- + + "getPhysicalOpId" should "extract operator id and layer name from a worker actor name" in { + val actor = ActorVirtualIdentity("Worker:WF7-myOp-main-3") + val opId = VirtualIdentityUtils.getPhysicalOpId(actor) + opId.logicalOpId.id shouldBe "myOp" + opId.layerName shouldBe "main" + } + + it should "fall back to __DummyOperator/__DummyLayer for non-worker actor names" in { + val controller = ActorVirtualIdentity("CONTROLLER") + val opId = VirtualIdentityUtils.getPhysicalOpId(controller) + opId.logicalOpId.id shouldBe "__DummyOperator" + opId.layerName shouldBe "__DummyLayer" + } + + it should "tolerate operator names that contain hyphens by greedy backtracking" in { + // The operator capture group is `.+` which backtracks to leave the trailing + // `-(\w+)-(\d+)` slots populated. A multi-hyphen operator name must still + // round-trip without losing characters from the operator itself. + val actor = ActorVirtualIdentity("Worker:WF1-multi-part-op-main-0") + val opId = VirtualIdentityUtils.getPhysicalOpId(actor) + opId.logicalOpId.id shouldBe "multi-part-op" + opId.layerName shouldBe "main" + } + + // ----- getWorkerIndex ----- + + "getWorkerIndex" should "return the trailing numeric workerId from a worker actor name" in { + val actor = ActorVirtualIdentity("Worker:WF7-myOp-main-42") + VirtualIdentityUtils.getWorkerIndex(actor) shouldBe 42 + } + + // Intentionally not covered: actor names that do not match workerNamePattern + // make getWorkerIndex throw scala.MatchError because the method has no + // fallback case. See the "Potential bug" note in the PR description. + Review Comment: The suite explicitly skips the only non-happy-path branch of `getWorkerIndex`, so the known `MatchError` on non-worker identities like `CONTROLLER`/`SELF` is still untested. Since this PR already documents that edge case, it would be better to pin it with a spec here so future changes can't silently change or reintroduce the behavior. ########## common/workflow-core/src/test/scala/org/apache/texera/amber/util/VirtualIdentityUtilsSpec.scala: ########## @@ -0,0 +1,133 @@ +/* + * 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.util + +import org.apache.texera.amber.core.virtualidentity.{ + ActorVirtualIdentity, + OperatorIdentity, + PhysicalOpIdentity, + WorkflowIdentity +} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +class VirtualIdentityUtilsSpec extends AnyFlatSpec with Matchers { + + // ----- createWorkerIdentity ----- + + "createWorkerIdentity (raw fields)" should "format Worker:WF<id>-<op>-<layer>-<workerIdx>" in { + val actor = VirtualIdentityUtils.createWorkerIdentity( + WorkflowIdentity(7), + operator = "myOp", + layerName = "main", + workerId = 3 + ) + actor.name shouldBe "Worker:WF7-myOp-main-3" + } + + "createWorkerIdentity (PhysicalOpIdentity overload)" should "delegate to the same encoded format" in { + val physicalOpId = PhysicalOpIdentity(OperatorIdentity("myOp"), "main") + val actor = VirtualIdentityUtils.createWorkerIdentity( + WorkflowIdentity(7), + physicalOpId, + workerId = 3 + ) + actor.name shouldBe "Worker:WF7-myOp-main-3" + } + + // ----- getPhysicalOpId ----- + + "getPhysicalOpId" should "extract operator id and layer name from a worker actor name" in { + val actor = ActorVirtualIdentity("Worker:WF7-myOp-main-3") + val opId = VirtualIdentityUtils.getPhysicalOpId(actor) + opId.logicalOpId.id shouldBe "myOp" + opId.layerName shouldBe "main" + } + + it should "fall back to __DummyOperator/__DummyLayer for non-worker actor names" in { + val controller = ActorVirtualIdentity("CONTROLLER") + val opId = VirtualIdentityUtils.getPhysicalOpId(controller) + opId.logicalOpId.id shouldBe "__DummyOperator" + opId.layerName shouldBe "__DummyLayer" + } + + it should "tolerate operator names that contain hyphens by greedy backtracking" in { + // The operator capture group is `.+` which backtracks to leave the trailing + // `-(\w+)-(\d+)` slots populated. A multi-hyphen operator name must still + // round-trip without losing characters from the operator itself. + val actor = ActorVirtualIdentity("Worker:WF1-multi-part-op-main-0") + val opId = VirtualIdentityUtils.getPhysicalOpId(actor) + opId.logicalOpId.id shouldBe "multi-part-op" + opId.layerName shouldBe "main" + } + + // ----- getWorkerIndex ----- + + "getWorkerIndex" should "return the trailing numeric workerId from a worker actor name" in { + val actor = ActorVirtualIdentity("Worker:WF7-myOp-main-42") + VirtualIdentityUtils.getWorkerIndex(actor) shouldBe 42 + } + + // Intentionally not covered: actor names that do not match workerNamePattern + // make getWorkerIndex throw scala.MatchError because the method has no + // fallback case. See the "Potential bug" note in the PR description. + + // ----- toShorterString ----- + + "toShorterString" should "keep operator names <= 6 chars unchanged" in { + val actor = ActorVirtualIdentity("Worker:WF1-myOp-main-0") + VirtualIdentityUtils.toShorterString(actor) shouldBe "WF1-myOp-main-0" Review Comment: This example uses `"myOp"`, so it only proves the `< 6` case even though the spec name says `<= 6 chars`. A regression from `operatorName.length > 6` to `>= 6` would still pass this test, so we should either use a six-character operator here or add an explicit boundary case. ########## common/workflow-core/src/test/scala/org/apache/texera/amber/util/VirtualIdentityUtilsSpec.scala: ########## @@ -0,0 +1,133 @@ +/* + * 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.util + +import org.apache.texera.amber.core.virtualidentity.{ + ActorVirtualIdentity, + OperatorIdentity, + PhysicalOpIdentity, + WorkflowIdentity +} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +class VirtualIdentityUtilsSpec extends AnyFlatSpec with Matchers { + + // ----- createWorkerIdentity ----- + + "createWorkerIdentity (raw fields)" should "format Worker:WF<id>-<op>-<layer>-<workerIdx>" in { + val actor = VirtualIdentityUtils.createWorkerIdentity( + WorkflowIdentity(7), + operator = "myOp", + layerName = "main", + workerId = 3 + ) + actor.name shouldBe "Worker:WF7-myOp-main-3" + } + + "createWorkerIdentity (PhysicalOpIdentity overload)" should "delegate to the same encoded format" in { + val physicalOpId = PhysicalOpIdentity(OperatorIdentity("myOp"), "main") + val actor = VirtualIdentityUtils.createWorkerIdentity( + WorkflowIdentity(7), + physicalOpId, + workerId = 3 + ) + actor.name shouldBe "Worker:WF7-myOp-main-3" + } + + // ----- getPhysicalOpId ----- + + "getPhysicalOpId" should "extract operator id and layer name from a worker actor name" in { + val actor = ActorVirtualIdentity("Worker:WF7-myOp-main-3") + val opId = VirtualIdentityUtils.getPhysicalOpId(actor) + opId.logicalOpId.id shouldBe "myOp" + opId.layerName shouldBe "main" + } + + it should "fall back to __DummyOperator/__DummyLayer for non-worker actor names" in { + val controller = ActorVirtualIdentity("CONTROLLER") + val opId = VirtualIdentityUtils.getPhysicalOpId(controller) + opId.logicalOpId.id shouldBe "__DummyOperator" + opId.layerName shouldBe "__DummyLayer" + } + + it should "tolerate operator names that contain hyphens by greedy backtracking" in { + // The operator capture group is `.+` which backtracks to leave the trailing + // `-(\w+)-(\d+)` slots populated. A multi-hyphen operator name must still + // round-trip without losing characters from the operator itself. + val actor = ActorVirtualIdentity("Worker:WF1-multi-part-op-main-0") + val opId = VirtualIdentityUtils.getPhysicalOpId(actor) + opId.logicalOpId.id shouldBe "multi-part-op" + opId.layerName shouldBe "main" + } + Review Comment: This suite adds a hyphen-handling regression test for the operator segment, but it still never exercises worker IDs whose *layer* contains hyphens. Those layer names already exist in the codebase (for example `"1st-physical-op"` / `"2nd-physical-op"` in `amber/.../WorkerSpec.scala`), and `workerNamePattern` is shared by `getPhysicalOpId`, `getWorkerIndex`, and `toShorterString`. Without a test for that shape, the current misparse of hyphenated layers remains undetected. -- 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]
