pierrejeambrun commented on code in PR #27987:
URL: https://github.com/apache/airflow/pull/27987#discussion_r1036324887
##########
airflow/www/static/js/api/useDatasetDependencies.ts:
##########
@@ -82,111 +85,89 @@ const generateGraph = ({ nodes, edges, font }:
GenerateProps) => ({
edges: edges.map((e) => ({ id: `${e.source}-${e.target}`, sources:
[e.source], targets: [e.target] })),
});
-interface SeparateGraphsProps extends DatasetDependencies {
- graphs: DatasetDependencies[];
+interface SeparateGraphsProps {
+ edges: DepEdge[];
+ graphs: EdgeGroup[];
}
-const graphIndicesToMerge: Record<number, number[]> = {};
-const indicesToRemove: number[] = [];
-
// find the downstream graph of each upstream edge
const findDownstreamGraph = (
- { edges, nodes, graphs = [] }: SeparateGraphsProps,
-): DatasetDependencies[] => {
- const newGraphs = [...graphs];
+ { edges, graphs = [] }: SeparateGraphsProps,
+): EdgeGroup[] => {
let filteredEdges = [...edges];
- graphs.forEach((g, i) => {
- // find downstream edges
- const downstreamEdges = edges.filter((e) => g.edges.some((ge) => ge.target
=== e.source));
- const downstreamNodes: DepNode[] = [];
-
- downstreamEdges.forEach((e) => {
- const newNode = nodes.find((n) => n.id === e.target);
- if (newNode) {
- downstreamNodes.push(newNode);
-
- // check if the node already exists in a different graph
- const existingGraphIndex = newGraphs
- .findIndex(((ng) => ng.nodes.some((n) => n.id === newNode.id)));
-
- // mark if the graph needs to merge with another
- if (existingGraphIndex > -1) {
- indicesToRemove.push(existingGraphIndex);
- graphIndicesToMerge[i] = [...(graphIndicesToMerge[i] || []),
existingGraphIndex];
- }
-
- // add node and edge to the graph
- newGraphs[i] = {
- nodes: [...newGraphs[i].nodes, newNode],
- edges: [...newGraphs[i].edges, e],
- };
-
- // remove edge from edge list
- filteredEdges = filteredEdges
- .filter((fe) => !(fe.source === e.source && fe.target === e.target));
+ // merge graphs that share edges
+ const mergedGraphs = graphs.reduce(
+ (newGraphs, g, i) => {
+ const otherGroupIndex = newGraphs.findIndex(
+ (og) => og.edges.some(
+ (oge) => g.edges.some(
+ (e) => e.target === oge.target,
+ ),
+ ),
+ );
+ if (otherGroupIndex === -1) {
+ return [...newGraphs, g];
}
+ const mergedEdges = [...newGraphs[otherGroupIndex].edges, ...g.edges]
+ .filter((e, j, totalEdges) => (
+ j === totalEdges.findIndex((t) => t.source === e.source && t.target
=== e.target)
+ ));
+ return [
+ ...newGraphs.filter((_, k) => k !== i && k !== otherGroupIndex),
Review Comment:
Not sure to undestand the left condition here (k !== i).
##########
airflow/www/static/js/api/useDatasetDependencies.ts:
##########
@@ -82,111 +85,89 @@ const generateGraph = ({ nodes, edges, font }:
GenerateProps) => ({
edges: edges.map((e) => ({ id: `${e.source}-${e.target}`, sources:
[e.source], targets: [e.target] })),
});
-interface SeparateGraphsProps extends DatasetDependencies {
- graphs: DatasetDependencies[];
+interface SeparateGraphsProps {
+ edges: DepEdge[];
+ graphs: EdgeGroup[];
}
-const graphIndicesToMerge: Record<number, number[]> = {};
-const indicesToRemove: number[] = [];
-
// find the downstream graph of each upstream edge
const findDownstreamGraph = (
- { edges, nodes, graphs = [] }: SeparateGraphsProps,
-): DatasetDependencies[] => {
- const newGraphs = [...graphs];
+ { edges, graphs = [] }: SeparateGraphsProps,
+): EdgeGroup[] => {
let filteredEdges = [...edges];
- graphs.forEach((g, i) => {
- // find downstream edges
- const downstreamEdges = edges.filter((e) => g.edges.some((ge) => ge.target
=== e.source));
- const downstreamNodes: DepNode[] = [];
-
- downstreamEdges.forEach((e) => {
- const newNode = nodes.find((n) => n.id === e.target);
- if (newNode) {
- downstreamNodes.push(newNode);
-
- // check if the node already exists in a different graph
- const existingGraphIndex = newGraphs
- .findIndex(((ng) => ng.nodes.some((n) => n.id === newNode.id)));
-
- // mark if the graph needs to merge with another
- if (existingGraphIndex > -1) {
- indicesToRemove.push(existingGraphIndex);
- graphIndicesToMerge[i] = [...(graphIndicesToMerge[i] || []),
existingGraphIndex];
- }
-
- // add node and edge to the graph
- newGraphs[i] = {
- nodes: [...newGraphs[i].nodes, newNode],
- edges: [...newGraphs[i].edges, e],
- };
-
- // remove edge from edge list
- filteredEdges = filteredEdges
- .filter((fe) => !(fe.source === e.source && fe.target === e.target));
+ // merge graphs that share edges
+ const mergedGraphs = graphs.reduce(
+ (newGraphs, g, i) => {
+ const otherGroupIndex = newGraphs.findIndex(
+ (og) => og.edges.some(
Review Comment:
This is hard to understand. Maybe more explicit names for og, oge and e
could help. Just an idea, maybe `graph`, `graph_edge`, `g_edge` ?
##########
airflow/www/static/js/api/useDatasetDependencies.ts:
##########
@@ -82,111 +85,89 @@ const generateGraph = ({ nodes, edges, font }:
GenerateProps) => ({
edges: edges.map((e) => ({ id: `${e.source}-${e.target}`, sources:
[e.source], targets: [e.target] })),
});
-interface SeparateGraphsProps extends DatasetDependencies {
- graphs: DatasetDependencies[];
+interface SeparateGraphsProps {
+ edges: DepEdge[];
+ graphs: EdgeGroup[];
}
-const graphIndicesToMerge: Record<number, number[]> = {};
-const indicesToRemove: number[] = [];
-
// find the downstream graph of each upstream edge
const findDownstreamGraph = (
- { edges, nodes, graphs = [] }: SeparateGraphsProps,
-): DatasetDependencies[] => {
- const newGraphs = [...graphs];
+ { edges, graphs = [] }: SeparateGraphsProps,
+): EdgeGroup[] => {
let filteredEdges = [...edges];
- graphs.forEach((g, i) => {
- // find downstream edges
- const downstreamEdges = edges.filter((e) => g.edges.some((ge) => ge.target
=== e.source));
- const downstreamNodes: DepNode[] = [];
-
- downstreamEdges.forEach((e) => {
- const newNode = nodes.find((n) => n.id === e.target);
- if (newNode) {
- downstreamNodes.push(newNode);
-
- // check if the node already exists in a different graph
- const existingGraphIndex = newGraphs
- .findIndex(((ng) => ng.nodes.some((n) => n.id === newNode.id)));
-
- // mark if the graph needs to merge with another
- if (existingGraphIndex > -1) {
- indicesToRemove.push(existingGraphIndex);
- graphIndicesToMerge[i] = [...(graphIndicesToMerge[i] || []),
existingGraphIndex];
- }
-
- // add node and edge to the graph
- newGraphs[i] = {
- nodes: [...newGraphs[i].nodes, newNode],
- edges: [...newGraphs[i].edges, e],
- };
-
- // remove edge from edge list
- filteredEdges = filteredEdges
- .filter((fe) => !(fe.source === e.source && fe.target === e.target));
+ // merge graphs that share edges
+ const mergedGraphs = graphs.reduce(
+ (newGraphs, g, i) => {
+ const otherGroupIndex = newGraphs.findIndex(
+ (og) => og.edges.some(
+ (oge) => g.edges.some(
+ (e) => e.target === oge.target,
+ ),
+ ),
+ );
+ if (otherGroupIndex === -1) {
+ return [...newGraphs, g];
}
+ const mergedEdges = [...newGraphs[otherGroupIndex].edges, ...g.edges]
Review Comment:
```typescript
// merge edges and remove duplicates
```
It is also done 2 times (the remove duplicate edges part), maybe extratinct
this to its own little function could help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
