Skip to content

Commit

Permalink
Fix CanonicalPlanGenerate to canonicalize plans without the plan ID
Browse files Browse the repository at this point in the history
  • Loading branch information
jaystarshot committed Mar 4, 2024
1 parent 5afd695 commit aadfba5
Showing 1 changed file with 25 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.facebook.presto.spi.plan.OrderingScheme;
import com.facebook.presto.spi.plan.OutputNode;
import com.facebook.presto.spi.plan.PlanNode;
import com.facebook.presto.spi.plan.PlanNodeId;
import com.facebook.presto.spi.plan.PlanNodeIdAllocator;
import com.facebook.presto.spi.plan.ProjectNode;
import com.facebook.presto.spi.plan.TableScanNode;
Expand Down Expand Up @@ -107,7 +108,10 @@
public class CanonicalPlanGenerator
extends InternalPlanVisitor<Optional<PlanNode>, CanonicalPlanGenerator.Context>
{
private final PlanNodeIdAllocator planNodeidAllocator = new PlanNodeIdAllocator();
private static final String CANONICAL_STRING = "CANONICAL";

// Not using a new override to objectMapper because PlanNodeId has a JsonValue annotation which cannot be directly overriden in a serializer
private final PlanNodeIdAllocator planNodeidAllocator;
private final VariableAllocator variableAllocator = new VariableAllocator();
// TODO: DEFAULT strategy has a very different canonicalizaiton implementation, refactor it into a separate class.
private final PlanCanonicalizationStrategy strategy;
Expand All @@ -119,6 +123,26 @@ public CanonicalPlanGenerator(PlanCanonicalizationStrategy strategy, ObjectMappe
this.strategy = requireNonNull(strategy, "strategy is null");
this.objectMapper = requireNonNull(objectMapper, "objectMapper is null");
this.session = requireNonNull(session, "session is null");
this.planNodeidAllocator = createPlanNodeIdAllocator(strategy);
}

private PlanNodeIdAllocator createPlanNodeIdAllocator(PlanCanonicalizationStrategy strategy)
{
//ToDO: For HBO we always want planNodeId to be canonicalized but currently fragment result caching is using the same class with default strategy
// refactor the default strategy to a different class
if (strategy.equals(DEFAULT)) {
return new PlanNodeIdAllocator();
}
else {
return new PlanNodeIdAllocator()
{
@Override
public PlanNodeId getNextId()
{
return new PlanNodeId(CANONICAL_STRING);
}
};
}
}

public static Optional<CanonicalPlanFragment> generateCanonicalPlanFragment(PlanNode root, PartitioningScheme partitioningScheme, ObjectMapper objectMapper, Session session)
Expand Down

0 comments on commit aadfba5

Please sign in to comment.