Skip to content

Commit

Permalink
Fix a bug where a cte producer was dropped due to errors creating the…
Browse files Browse the repository at this point in the history
… cloned cte dependency graph in SimplifyPlanWithEmptyInput optimizer

Fixed a bug in removeCteProducersFromCteDependencyGraph function of Sequence node, currently used by SimplifyPlanWithEmptyInput optimizer where a cte producer was dropped from the graph (and from scheduling) if there were no edges leading from it leading to wrong results
  • Loading branch information
jaystarshot committed Apr 9, 2024
1 parent 0e31914 commit 323d587
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,18 @@ public void testCteExecutionWhereOneCteRemovedBySimplifyEmptyInputRule()
sql));
}

@Test
public void testCteExecutionWhereChildPlanRemovedBySimplifyEmptyInputRule()
{
String sql = "WITH t as(SELECT * FROM orders LEFT JOIN (select orderkey from orders where false) ON TRUE) " +
"SELECT * FROM t";
QueryRunner queryRunner = getQueryRunner();
compareResults(queryRunner.execute(getMaterializedSession(),
sql),
queryRunner.execute(getSession(),
sql));
}

@Test
public void testSimplePersistentCte()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

import static com.google.common.base.Preconditions.checkArgument;

Expand Down Expand Up @@ -69,6 +70,9 @@ public SequenceNode(Optional<SourceLocation> sourceLocation,
this.cteProducers = ImmutableList.copyOf(cteProducerList);
this.primarySource = primarySource;
checkArgument(cteDependencyGraph.isDirected(), "Sequence Node expects a directed graph");
IntStream.range(0, cteProducerList.size())
.forEach(i -> checkArgument(cteDependencyGraph.nodes().contains(i),
"Sequence Node Error: The cteProducer at index " + i + " is missing in the cte dependency graph."));
this.cteDependencyGraph = ImmutableGraph.copyOf(cteDependencyGraph);
}

Expand Down Expand Up @@ -121,6 +125,9 @@ public Graph<Integer> getCteDependencyGraph()
// Returns a Graph after removing indexes
public Graph<Integer> removeCteProducersFromCteDependencyGraph(Set<Integer> indexesToRemove)
{
if (indexesToRemove.isEmpty()) {
return ImmutableGraph.copyOf(getCteDependencyGraph());
}
Graph<Integer> originalGraph = getCteDependencyGraph();
MutableGraph newCteDependencyGraph = GraphBuilder.from(getCteDependencyGraph()).build();
Map<Integer, Integer> indexMapping = new HashMap<>();
Expand All @@ -138,6 +145,7 @@ public Graph<Integer> removeCteProducersFromCteDependencyGraph(Set<Integer> inde
for (int oldIndex : originalGraph.nodes()) {
if (!indexesToRemove.contains(oldIndex)) {
Integer newIndex = indexMapping.get(oldIndex);
newCteDependencyGraph.addNode(newIndex);
for (Integer successor : originalGraph.successors(oldIndex)) {
if (!indexesToRemove.contains(successor)) {
Integer newSuccessorIndex = indexMapping.get(successor);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.graph.GraphBuilder;
import com.google.common.graph.MutableGraph;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
Expand Down Expand Up @@ -528,12 +529,16 @@ public void testSequence()
cteConsumerNode2,
JoinDistributionType.PARTITIONED,
"orderkey", "custkey");
MutableGraph<Integer> sequenceGraph = GraphBuilder.directed().build();
// Add indexes to the graph
sequenceGraph.addNode(0);
sequenceGraph.addNode(1);
SequenceNode sequenceNode = new SequenceNode(
Optional.empty(),
new PlanNodeId("sequence"),
ImmutableList.of(cteProducerNode1, cteProducerNode2),
joinNode,
GraphBuilder.directed().build());
sequenceGraph);

// Define cost of sequence children
Map<String, PlanCostEstimate> costs = ImmutableMap.of(
Expand Down

0 comments on commit 323d587

Please sign in to comment.