fix: support new HugeGraph edge id format#349

Open

hutiefang76 wants to merge 5 commits into

hutiefang76:codex/fix-hugegraph-edge-id-format

Open

fix: support new HugeGraph edge id format #349
hutiefang76 wants to merge 5 commits into
apache:master from
hutiefang76:codex/fix-hugegraph-edge-id-format

Conversation


 fix: support new HugeGraph edge id format

27b727a

@dosubot dosubot Bot added the size:M label

Jun 21, 2026

imbajin

imbajin reviewed

Jun 21, 2026

View reviewed changes

@imbajin imbajin left a comment

Copy link

Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is in the right area, but the compatibility logic should mirror the java-client edge-id semantics more directly and lock the legacy path with a test.

...uter/computer-core/src/main/java/org/apache/hugegraph/computer/core/input/HugeConverter.java Outdated

}

String[] parts = SplicingIdGenerator.split(edgeId);

if (parts.length == 4) {

@imbajin imbajin Jun 21, 2026 •

edited

Loading

Copy link

Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❗️ 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() 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:

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.

@imbajin

imbajin commented Jun 21, 2026

Copy link

Copy Markdown

Member

❗️ Please also update the CI HugeGraph environment after fixing the parser.

🔗 Reference: computer-ci.yml

Context

The current workflow still runs integration tests with GRAPH_ENV_VERSION: 1.3.0.
The adjacent TODO still says to adapt Server/Loader to 1.5.0, but the current release line is already 1.7.0.
GRAPH_ENV_VERSION is passed into load-data-into-hugegraph.sh, which starts both:
- hugegraph/hugegraph:${GRAPH_ENV_VERSION}
- hugegraph/loader:${GRAPH_ENV_VERSION}

Required update

Please update the workflow to use the latest 1.7.0 HugeGraph Server/Loader images and remove the stale TODO, for example:

GRAPH_ENV_VERSION: 1.7.0

Test completeness

After that, the related coverage should prove both compatibility directions:

unit tests cover legacy 4-part edge ids;
unit tests cover current 5/6-part edge ids;
CI integration tests actually load data through HugeGraph Server/Loader 1.7.0, so this PR is validated against the permanent edge-id format rather than only the old 1.3.0 environment.


 fix: align edge id coverage with current HugeGraph

91a04d9

@hutiefang76

hutiefang76 commented Jun 21, 2026

Copy link

Copy Markdown

Author

Updated in 91a04d9:

Kept legacy 4-part edge ids on the existing Edge.name() path.
Added coverage for legacy 4-part ids, current 5-part ids, and both EDGE_OUT / EDGE_IN 6-part ids.
Switched GRAPH_ENV_VERSION to 1.7.0 and removed the stale 1.5.0 TODO.

Local checks:

mvn -pl computer-test -am -Djacoco.skip=true -Dtest=HugeConverterTest -Dsurefire.failIfNoSpecifiedTests=false test
git diff --check -- .github/workflows/computer-ci.yml computer/computer-core/src/main/java/org/apache/hugegraph/computer/core/input/HugeConverter.java computer/computer-test/src/main/java/org/apache/hugegraph/computer/core/input/HugeConverterTest.java

The new GitHub Actions runs are currently waiting for workflow approval (action_required).

imbajin

imbajin reviewed

Jun 22, 2026

View reviewed changes

@imbajin imbajin left a comment •

edited

Loading

Copy link

Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: Legacy edge-id compatibility still depends on client-version behavior, and latest Computer CI is cancelled. Evidence: JDK 11 HugeConverterTest passed locally; current-head computer-ci cancelled.

🔗 Please check the cancelled current-head Computer CI run: https://github.com/apache/hugegraph-computer/actions/runs/27905173009/job/82572760266

...uter/computer-core/src/main/java/org/apache/hugegraph/computer/core/input/HugeConverter.java Outdated

String[] parts = SplicingIdGenerator.split(edgeId);

if (parts.length == LEGACY_EDGE_ID_PARTS) {

return edge.name();

@imbajin imbajin Jun 22, 2026

Copy link

Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Keep legacy id parsing inside the shim

convertEdgeName() already splits the id, but the 4-part branch still delegates to edge.name(). That only works with the current hugegraph-client 1.3.0 dependency; the current java-client implementation only accepts 5/6-part ids and derives the name from idParts[idParts.length - 2], so this compatibility shim will break for legacy ids when the client dependency is aligned with the 1.7.0 runtime that this PR now validates against.

Please return parts[2] for LEGACY_EDGE_ID_PARTS directly, or use the shared parts[parts.length - 2] invariant for all accepted arities, and leave edge.name() only for null or unknown formats.

hutiefang added 2 commits

June 22, 2026 19:33


 fix: parse legacy edge names from edge ids

252ea40


 fix: tolerate current edge label schema fields

b84879d

@dosubot dosubot Bot added size:L and removed size:M labels

Jun 22, 2026

imbajin

imbajin previously approved these changes

Jun 22, 2026

View reviewed changes

@imbajin imbajin left a comment

Copy link

Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: no. Summary: No obvious issues found in the current head. Evidence: git diff --check passed; HugeConverterTest and HugeClientCompatibilityTest passed (8 tests); latest gh pr checks reported no checks.

@dosubot dosubot Bot added the lgtm label

Jun 22, 2026

@codecov

codecov Bot commented Jun 23, 2026

Copy link

Copy Markdown

Codecov Report

❌ Patch coverage is 84.61538% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.98%. Comparing base (e0b484a) to head (b84879d).
⚠️ Report is 39 commits behind head on master.

Files with missing lines	Patch %	Lines
...e/hugegraph/computer/core/input/HugeConverter.java	57.14%	2 Missing and 4 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@
## master #349 +/- ##
============================================
- Coverage 85.03% 84.98% -0.05% 
- Complexity 3296 3375 +79 
============================================
 Files 349 361 +12 
 Lines 12485 12716 +231 
 Branches 1130 1157 +27 
============================================
+ Hits 10616 10807 +191 
- Misses 1329 1340 +11 
- Partials 540 569 +29

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.


 test: cover edge name fallback paths

bc84f0a

@hutiefang76 hutiefang76 dismissed imbajin’s stale review via

bc84f0a

June 23, 2026 12:28

@hutiefang76

hutiefang76 commented Jun 23, 2026 •

edited

Loading

Copy link

Copy Markdown

Author

I checked the current computer-ci failure. The HeartbeatHandlerTest failure is reproducible on upstream master with the same JDK 11 container command, so it does not look specific to this edge-id change.

Command used on both this branch and upstream/master:

docker run --rm -v <repo>:/workspace -w /workspace maven:3.8.8-eclipse-temurin-11 \
 mvn -f computer/pom.xml -pl computer-test -am -P unit-test \
 -Dtest=NettyTransportClientTest,HeartbeatHandlerTest \
 -Djacoco.skip=true -Dsurefire.failIfNoSpecifiedTests=false test

Result:

NettyTransportClientTest passed locally in the JDK 11 container.
HeartbeatHandlerTest.testHeartbeatHandler failed on both branches with Wanted 4 times ... But was 3 times.

I will keep this PR scoped to the edge-id compatibility fix unless you prefer me to include a separate flaky-test stabilization change here.

Labels

lgtm size:L

2 participants

@hutiefang76 @imbajin

Conversation

@hutiefang76 hutiefang76 commented Jun 21, 2026

Purpose of the PR

Main Changes

Verifying these changes

Does this PR potentially affect the following parts?

Documentation Status

Uh oh!

@imbajin imbajin left a comment

Choose a reason for hiding this comment

Uh oh!

@imbajin imbajin Jun 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imbajin commented Jun 21, 2026

Uh oh!

hutiefang76 commented Jun 21, 2026

Uh oh!

@imbajin imbajin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@imbajin imbajin Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

@imbajin imbajin left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jun 23, 2026

Codecov Report

Uh oh!

hutiefang76 commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

@imbajin imbajin Jun 21, 2026 •

edited

Loading

@imbajin imbajin left a comment •

edited

Loading

hutiefang76 commented Jun 23, 2026 •

edited

Loading