Skip to content

Commit 83d7a13

Browse files
committed
fix: suggestions from code review
1 parent 2a88d99 commit 83d7a13

2 files changed

Lines changed: 72 additions & 5 deletions

File tree

libs/@local/hashql/mir/src/pass/execution/island/graph/mod.rs

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -466,11 +466,20 @@ impl<'graph, A: Allocator, S: Allocator + Clone> RequirementResolver<'graph, A,
466466
self.graph[provider].provides.insert(path);
467467
}
468468

469-
self.graph.inner.add_edge(
470-
NodeId::from_u32(provider.as_u32()),
471-
NodeId::from_u32(consumer.as_u32()),
472-
IslandEdge::DataFlow,
473-
);
469+
let provider_node = NodeId::from_u32(provider.as_u32());
470+
let consumer_node = NodeId::from_u32(consumer.as_u32());
471+
472+
let exists = self
473+
.graph
474+
.inner
475+
.outgoing_edges(provider_node)
476+
.any(|edge| edge.target() == consumer_node && edge.data == IslandEdge::DataFlow);
477+
478+
if !exists {
479+
self.graph
480+
.inner
481+
.add_edge(provider_node, consumer_node, IslandEdge::DataFlow);
482+
}
474483
}
475484

476485
/// Returns an existing data island for the given origin backend, or creates one.

libs/@local/hashql/mir/src/pass/execution/island/graph/tests.rs

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,64 @@ fn entry_island_needs_fetch() {
469469
assert!(provides.contains(EntityPath::Properties));
470470
}
471471

472+
/// DataFlow edge dedup: an Interpreter island accesses two paths that both originate
473+
/// from Postgres (`Properties` and `EntityUuid`). Both resolve to the same Postgres
474+
/// dominator, but only one `DataFlow` edge should exist between them.
475+
#[test]
476+
fn data_flow_edge_dedup() {
477+
let heap = Heap::new();
478+
let interner = Interner::new(&heap);
479+
let env = Environment::new(&heap);
480+
481+
let body = body!(interner, env; [graph::read::filter]@0/2 -> ? {
482+
decl env: (), vertex: [Opaque sym::path::Entity; ?], val1: ?, val2: ?;
483+
@proj props = vertex.properties: ?,
484+
meta = vertex.metadata: ?,
485+
rec = meta.record_id: ?,
486+
eid = rec.entity_id: ?,
487+
uuid = eid.entity_uuid: ?;
488+
489+
bb0() {
490+
val1 = load props;
491+
goto bb1();
492+
},
493+
bb1() {
494+
val1 = load props;
495+
val2 = load uuid;
496+
return val1;
497+
}
498+
});
499+
500+
let graph = build_graph(&body, &[TargetId::Postgres, TargetId::Interpreter]);
501+
assert_eq!(graph.node_count(), 2);
502+
503+
let postgres = find_island(&graph, TargetId::Postgres);
504+
let interpreter = find_island(&graph, TargetId::Interpreter);
505+
506+
// Postgres provides both paths to the Interpreter island.
507+
let provides = graph[postgres].provides();
508+
let provides = provides.as_entity().expect("entity vertex");
509+
assert!(provides.contains(EntityPath::Properties));
510+
assert!(provides.contains(EntityPath::EntityUuid));
511+
assert_eq!(provides.len(), 2);
512+
513+
// Exactly two edges: one ControlFlow and one DataFlow, both Postgres -> Interpreter.
514+
// Without dedup, the two paths would produce two DataFlow edges to the same consumer.
515+
assert_eq!(graph.edge_count(), 2);
516+
assert!(has_edge(
517+
&graph,
518+
postgres,
519+
interpreter,
520+
IslandEdge::ControlFlow
521+
));
522+
assert!(has_edge(
523+
&graph,
524+
postgres,
525+
interpreter,
526+
IslandEdge::DataFlow
527+
));
528+
}
529+
472530
/// Control flow edge dedup: two blocks in the same island both have a successor in
473531
/// another island, but only one `ControlFlow` edge should be created.
474532
#[test]

0 commit comments

Comments
 (0)