fix(visualization): take the union of Network Graph's two node columns - #7327
Merged
Conversation
Contributor
Backport auto-label reportThis
|
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7327 +/- ##
============================================
+ Coverage 83.37% 83.42% +0.04%
- Complexity 4129 4139 +10
============================================
Files 1166 1167 +1
Lines 46424 46442 +18
Branches 5174 5174
============================================
+ Hits 38707 38745 +38
+ Misses 6000 5980 -20
Partials 1717 1717
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 379 | 0.231 | 25,556/34,025/34,025 us | 🔴 -7.6% / 🔴 +114.0% |
| 🔴 | bs=100 sw=10 sl=64 | 781 | 0.477 | 126,237/153,415/153,415 us | 🔴 +11.0% / 🔴 +39.9% |
| ⚪ | bs=1000 sw=10 sl=64 | 899 | 0.549 | 1,116,801/1,163,082/1,163,082 us | ⚪ within ±5% / 🔴 +11.3% |
Baseline details
Latest main f46b2a7 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 379 tuples/sec | 409 tuples/sec | 762.27 tuples/sec | -7.3% | -50.3% |
| bs=10 sw=10 sl=64 | MB/s | 0.231 MB/s | 0.25 MB/s | 0.465 MB/s | -7.6% | -50.3% |
| bs=10 sw=10 sl=64 | p50 | 25,556 us | 23,752 us | 12,866 us | +7.6% | +98.6% |
| bs=10 sw=10 sl=64 | p95 | 34,025 us | 34,076 us | 15,903 us | -0.2% | +114.0% |
| bs=10 sw=10 sl=64 | p99 | 34,025 us | 34,076 us | 19,260 us | -0.2% | +76.7% |
| bs=100 sw=10 sl=64 | throughput | 781 tuples/sec | 825 tuples/sec | 966.64 tuples/sec | -5.3% | -19.2% |
| bs=100 sw=10 sl=64 | MB/s | 0.477 MB/s | 0.504 MB/s | 0.59 MB/s | -5.4% | -19.2% |
| bs=100 sw=10 sl=64 | p50 | 126,237 us | 120,479 us | 103,594 us | +4.8% | +21.9% |
| bs=100 sw=10 sl=64 | p95 | 153,415 us | 138,255 us | 109,677 us | +11.0% | +39.9% |
| bs=100 sw=10 sl=64 | p99 | 153,415 us | 138,255 us | 120,247 us | +11.0% | +27.6% |
| bs=1000 sw=10 sl=64 | throughput | 899 tuples/sec | 898 tuples/sec | 998.67 tuples/sec | +0.1% | -10.0% |
| bs=1000 sw=10 sl=64 | MB/s | 0.549 MB/s | 0.548 MB/s | 0.61 MB/s | +0.2% | -9.9% |
| bs=1000 sw=10 sl=64 | p50 | 1,116,801 us | 1,111,123 us | 1,009,199 us | +0.5% | +10.7% |
| bs=1000 sw=10 sl=64 | p95 | 1,163,082 us | 1,187,112 us | 1,045,335 us | -2.0% | +11.3% |
| bs=1000 sw=10 sl=64 | p99 | 1,163,082 us | 1,187,112 us | 1,076,869 us | -2.0% | +8.0% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,527.47,200,128000,379,0.231,25556.35,34024.65,34024.65
1,100,10,64,20,2560.05,2000,1280000,781,0.477,126237.41,153414.96,153414.96
2,1000,10,64,20,22248.02,20000,12800000,899,0.549,1116800.69,1163081.63,1163081.63
The node set came from 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 columns, and those glued values were added to the graph as nodes; the genuine nodes only arrived afterwards, with the edges. Every graph therefore carried one unconnected dot per distinct source-destination pair, reporting zero connections and indistinguishable from real data, and a pair of columns with different types aborted the run outright. Take the union instead, in first-appearance order: a set iterates strings in an order that varies between processes, so the node sequence would shift from run to run. Closes apache#7325 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kz930
force-pushed
the
fix-network-graph-node-set
branch
from
August 5, 2026 21:15
629384c to
4096382
Compare
Reverting the one-line change leaves this the only failing case, so the expression is held rather than merely written once. It also pins the ordered de-duplication: a set would satisfy "union" while reordering the nodes between processes, which is not what the operator needs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@carloea2 May you take a look at this? |
Contributor
|
Does this operator work on |
Contributor
Author
|
No, it's wrong on every input. sources + destinations adds the two columns row by row, so for rows n1→n2, n2→n3, n3→n4 the node set came out as n1n2, n2n3, n3n4 — not one real node. The graph still looked almost right only because adding the edges put the real nodes back, leaving those three as extra isolated dots: seven nodes instead of four. |
Contributor
|
@kz930 Screenshots? |
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 — asetiterates 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
+norsetis right here.Any related issues, documentation, discussions?
Closes #7325.
How was this PR tested?
NetworkGraphOpDescSpecgains 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 asetwould satisfy "union" while reordering the nodes between processes.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-n2andn2-to-n3. Before the change it produced seven nodes — the four real ones plusn3n4,n2n3andn1n2, each reporting zero connections, withn2n3sitting in the same picture as the genuine edge fromn2ton3. After it, four nodes with the correct connection counts and no isolated dots.Before the change:
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:
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)