deephaven / deephaven-core

Deephaven Community Core
Other
255 stars 80 forks source link

`NotificationQueue.Dependency#getUpdateGraph()` incorrect return value #4326

Open lbooker42 opened 1 year ago

lbooker42 commented 1 year ago

Description

The documentation for NotificationQueue.Dependency#getUpdateGraph() explicitly states: if all dependencies are non-refreshing dynamic nodes, return null. However, the function currently returns the updateGraph for the first table even when provided only static tables.

Expected results

Return null when all input tables are static.

Actual results

Returns the updateGraph for the first table even when all inputs are static.

Additional details and attachments

Code that corrects this problem is provided below. Unfortunately, there are many usages of this function that rely on the current (mis)behavior that need corrected before this code can be used.

        static UpdateGraph getUpdateGraph(@Nullable Dependency first, Dependency... dependencies) {
            UpdateGraph graph = null;

            if (first != null && DynamicNode.notDynamicOrIsRefreshing(first)) {
                graph = first.getUpdateGraph();
            }

            for (final Dependency other : dependencies) {
                if (other == null || DynamicNode.isDynamicAndNotRefreshing(other)) {
                    continue;
                }
                if (graph == null) {
                    graph = other.getUpdateGraph();
                } else if (graph != other.getUpdateGraph()) {
                    throw new UpdateGraphConflictException("Multiple update graphs found in dependencies: " + graph
                            + " and " + other.getUpdateGraph());
                }
            }

            return graph;
        }
    }

Versions

nbauernfeind commented 9 months ago

I suspect we might actually want to return something like a StaticUpdateGraph singleton that has functioning locks, but is otherwise a dummy impl.