Skip to content

fix(visualization, v1.2): take the union of Network Graph's two node columns - #7401

Closed
github-actions[bot] wants to merge 1 commit into
release/v1.2from
backport/7327-take-the-union-of-network-graph-s-two-no-v1.2
Closed

fix(visualization, v1.2): take the union of Network Graph's two node columns#7401
github-actions[bot] wants to merge 1 commit into
release/v1.2from
backport/7327-take-the-union-of-network-graph-s-two-no-v1.2

Conversation

@github-actions

@github-actions github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Automated backport of #7327 to release/v1.2.

Source: 357296d · automation run

Any related issues, documentation, discussions?

Backport of #7327. Originally linked #7325.

How was this PR tested?

Release-branch CI runs on this branch once the conflicts are resolved and this PR is marked ready for review.

Was this PR authored or co-authored using generative AI tooling?

No.

#7327)

### What changes were proposed in this PR?

Network Graph built its node set with `set(sources + destinations)`. On
two pandas Series `+` is element-wise, so the set held each source glued
to its destination rather than the union of the two columns; those glued
values were added to the graph as nodes, and the genuine nodes only
arrived afterwards with the edges. This takes the union instead, in
first-appearance order — a `set` iterates strings in an order that
varies between processes, which would leave the node sequence unstable
from run to run.

One line of code, plus a comment recording why neither `+` nor `set` is
right here.

### Any related issues, documentation, discussions?

Closes #7325.

### How was this PR tested?

`NetworkGraphOpDescSpec` gains a case asserting the node set is built as
a union: reverting the one-line change leaves it the only failing test.
It also pins the ordered de-duplication, since a `set` would satisfy
"union" while reordering the nodes between processes.

```
sbt "WorkflowOperator/testOnly org.apache.texera.amber.operator.visualization.networkGraph.NetworkGraphOpDescSpec"
```

Six cases, all passing.

Beyond that, the operator's generated module was dumped and executed
over ten rows carrying the edges `n3-to-n4`, `n1-to-n2` and `n2-to-n3`.
Before the change it produced seven nodes — the four real ones plus
`n3n4`, `n2n3` and `n1n2`, each reporting zero connections, with `n2n3`
sitting in the same picture as the genuine edge from `n2` to `n3`. After
it, four nodes with the correct connection counts and no isolated dots.
Before the change:
<img width="1530" height="1001" alt="Screenshot 2026-08-07 at 1 50
52 PM"
src="https://github.com/user-attachments/assets/357f3d44-90a1-466a-bc26-da719dc9dfe3"
/>

Selecting an integer column as the source and a string column as the
destination aborted the run with `TypeError: unsupported operand type(s)
for +: 'int' and 'str'` before the change and renders normally after it.
Before the change:
<img width="1532" height="1001" alt="Screenshot 2026-08-07 at 1 51
17 PM"
src="https://github.com/user-attachments/assets/674ed5f1-6019-48d5-9a82-818ad228acf0"
/>

### Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 5)

---------

(backported from commit 357296d)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

The cherry-pick conflicted and was committed with conflict markers. Resolve the conflicts on this branch, then mark this PR ready for review.

Conflicting files:

  • common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/networkGraph/NetworkGraphOpDescSpec.scala

@kz930

kz930 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Resolved in #7518, opened from a fork since I don't have write access to this branch.

Worth noting for anyone picking this up: NetworkGraphOpDescSpec.scala doesn't exist on release/v1.2, and the modify/delete here was resolved by taking the whole file from main. That brings across three cases from #5640, which wasn't backported — they assert the assertion text names the empty field, and this branch still has a bare assert(source.nonEmpty). Running this branch as it stands gives three passed, three failed. #7518 carries only the case #7327 added. Could a committer close this one?

@xuang7

xuang7 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Closing as a duplicate of #7518.

@xuang7 xuang7 closed this Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants