Skip to content

Commit

Permalink
clean(dag): cleanup unused Filter function
Browse files Browse the repository at this point in the history
Following 7ef5705 the "Filter" function is not used anymore.

After thinking I think it will not ever be used in this form as it is
not generic and thus it is better to completely delete it.

Indeed the function is buggy, difficult to understand (and to fix). E.g. I
still have not grasped what it try to do with the orphans and their
childs.

I think (without being sure) the idea behind this was to be able to work
on a smaller graph for performance but as the small graph is used just
once, the cost of constructing it in the first place completely offsets
the potential gains.
  • Loading branch information
joulaud committed Jan 13, 2025
1 parent f9453c8 commit 96fca0a
Show file tree
Hide file tree
Showing 2 changed files with 0 additions and 77 deletions.
51 changes: 0 additions & 51 deletions pkg/dag/dag.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,57 +107,6 @@ func (d *DAG) WalkParallel(visitor NodeVisitorFunc) {
waitGroup.Wait()
}

// Filter creates a new DAG only populated by nodes that fulfil the condition checked by filterFunc.
// All children nodes that no longer have any parents in the resulting graph will become a root node of the DAG.
func (d *DAG) Filter(filterFunc func(*Node) bool) *DAG {
filteredGraph := DAG{}
replacements := make(map[*Node]*Node)
var orphans []*Node

d.Walk(func(node *Node) {
if !filterFunc(node) {
// The node does not fulfil the condition, skip it.
return
}

// Create a replacement node containing the same Image.
newNode := NewNode(node.Image)
replacements[node] = newNode
// Check if the node is orphan
for _, parent := range node.Parents() {
if _, ok := replacements[parent]; ok {
// The node has at least one parent, nothing to do.
return
}
}
// The node no longer has parents, we add it to the list of orphans.
orphans = append(orphans, node)
})

for _, orphan := range orphans {
orphan.walkInDepth(createUniqueVisitor(func(node *Node) {
replacement, ok := replacements[node]
if !ok {
// This node on the source graph should not be in the filtered graph.
return
}

// Add new child nodes
for _, child := range node.Children() {
childReplacement, ok := replacements[child]
if !ok {
// This child node on the source graph should not be in the filtered graph.
continue
}
replacement.AddChild(childReplacement)
}
}))
// Add the orphan node to the root nodes of the graph.
filteredGraph.AddNode(replacements[orphan])
}

return &filteredGraph
}

Check failure on line 110 in pkg/dag/dag.go

View workflow job for this annotation

GitHub Actions / Run linters

File is not properly formatted (gofmt)
//nolint:musttag
func (d *DAG) ListImage() string {
Expand Down
26 changes: 0 additions & 26 deletions pkg/dag/dag_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,32 +214,6 @@ func Test_WalkParallel_RunsAllNodes(t *testing.T) {
assert.Equal(t, 6, length) // Total number of nodes is 6
}

func Test_Filter(t *testing.T) {
t.Parallel()

DAG := createDAG()

withoutParents := DAG.Filter(func(node *dag.Node) bool {
return len(node.Parents()) > 0 // Filter out nodes having parents.
})
assert.Len(t, withoutParents.Nodes(), 3)

withParents := DAG.Filter(func(node *dag.Node) bool {
return len(node.Parents()) == 0 // Filter out nodes having no parent.
})
assert.Len(t, withParents.Nodes(), 2)

withoutChildren := DAG.Filter(func(node *dag.Node) bool {
return len(node.Children()) > 0 // Filter out nodes having children.
})
assert.Len(t, withoutChildren.Nodes(), 2)

withChildren := DAG.Filter(func(node *dag.Node) bool {
return len(node.Children()) == 0 // Filter out nodes having no children.
})
assert.Len(t, withChildren.Nodes(), 3)
}

func Test_ListImage(t *testing.T) {
t.Parallel()

Expand Down

0 comments on commit 96fca0a

Please sign in to comment.