imbajin commented on code in PR #349:
URL: 
https://github.com/apache/hugegraph-computer/pull/349#discussion_r3448436267


##########
computer/computer-core/src/main/java/org/apache/hugegraph/computer/core/input/HugeConverter.java:
##########
@@ -96,4 +98,22 @@ public static Properties convertProperties(
         }
         return properties;
     }
+
+    public static String convertEdgeName(Edge edge) {
+        E.checkArgumentNotNull(edge, "The edge can't be null");
+        String edgeId = edge.id();
+        if (edgeId == null) {
+            return edge.name();
+        }
+
+        String[] parts = SplicingIdGenerator.split(edgeId);
+        if (parts.length == 4) {

Review Comment:
   ❗️ High priority: please align this parser with the java-client edge-id 
invariant instead of hardcoding each length/index pair.
   
   **Context**
   
   - Legacy client 1.3 parsed the old 4-part id as `parts[2]`.
   - Current toolchain java-client parses the permanent 5/6-part formats in 
[`Edge.name()`](https://github.com/apache/hugegraph-toolchain/blob/master/hugegraph-client/src/main/java/org/apache/hugegraph/structure/graph/Edge.java#L142-L149)
 as `idParts[idParts.length - 2]` after validating the part count.
   - So the stable semantic is: Computer's edge name is the edge sort-values 
segment, i.e. the penultimate part of a valid edge id.
   
   **Risk**
   
   The current implementation encodes the same rule as `4 -> parts[2]`, `5 -> 
parts[3]`, and `6 -> parts[4]`. That works for these examples, but it 
re-implements java-client parsing in a more fragile form and makes future 
format/client upgrades easier to drift.
   
   **Suggestion**
   
   Keep the compatibility range explicit, but extract through the shared 
invariant:
   
   ```java
   if (parts.length >= 4 && parts.length <= 6) {
       return parts[parts.length - 2];
   }
   ```
   
   Please also add a 4-part regression test beside the new 5/6-part tests, 
since this method explicitly preserves HugeGraph 1.3 compatibility but the 
current coverage only locks the new formats.



-- 
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]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to