Skip to content

Introduce conditional planner rules (ConditionalCascadesRule) #4332

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
RobertBrunel wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Introduce conditional planner rules (ConditionalCascadesRule) #4332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
184 changes: 136 additions & 48 deletions ...core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/CascadesPlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@
import com.apple.foundationdb.record.query.plan.cascades.matching.structure.ReferenceMatchers;
import com.apple.foundationdb.record.query.plan.plans.RecordQueryPlan;
import com.google.common.base.Suppliers;
import com.google.common.base.Verify;

Check notice on line 65 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/CascadesPlanner.java

View workflow job for this annotation

GitHub Actions / coverage 

File coverage: 88.5% (346/391 lines) | Changed lines: 66.2% (43/65 lines)
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -71,10 +72,10 @@
import java.util.ArrayDeque;
import java.util.Collection;
import java.util.Deque;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Function;
import java.util.function.Supplier;

Expand Down Expand Up @@ -544,7 +545,12 @@
@Nonnull
PlannerPhase getPlannerPhase();

void execute();
/**
* Executes this task, mutating the planner state (the memo and the task stack) as appropriate for the kind of
* task. The returned boolean reports whether the task made progress, that is, whether it changed planner state,
* for example by yielding new expressions, pushing planner requirements, or pushing more tasks onto the stack.
*/
boolean execute();

PlannerEvent toTaskEvent(Location location);
}
Expand Down Expand Up @@ -576,13 +582,14 @@
}

@Override
public void execute() {
public boolean execute() {
if (plannerPhase.hasNextPhase()) {
// if there is another phase push it first so it gets executed at the very end
taskStack.push(new InitiatePlannerPhase(plannerPhase.getNextPhase()));
}
taskStack.push(new OptimizeGroup(plannerPhase, currentRoot));
taskStack.push(new ExploreGroup(plannerPhase, currentRoot));
return true;
}

@Override
Expand Down Expand Up @@ -630,7 +637,7 @@
}

@Override
public void execute() {
public boolean execute() {
RelationalExpression bestFinalExpression = null;
final var costModel = plannerPhase.createCostModel(configuration);
for (final var finalExpression : group.getFinalExpressions()) {
Expand Down Expand Up @@ -671,6 +678,7 @@
} else {
group.pruneWith(bestFinalExpression);
}
return true;
}

@Override
Expand Down Expand Up @@ -713,13 +721,13 @@
}

@Override
public void execute() {
public boolean execute() {
final var targetPlannerStage = plannerPhase.getTargetPlannerStage();
final var groupPlannerStage = group.getPlannerStage();
if (targetPlannerStage != groupPlannerStage) {
if (targetPlannerStage.precedes(groupPlannerStage)) {
// group is further along in the planning process, do not re-explore
return;
return false;
} else {
//
// targetPlannerStage succeeds groupPlannerStage
Expand Down Expand Up @@ -753,6 +761,7 @@
} else {
group.commitExploration();
}
return true;
}

@Override
Expand Down Expand Up @@ -821,19 +830,19 @@
}

@Override
public void execute() {
public boolean execute() {
final var ruleSet = getPlannerPhase().getRuleSet();

// push all rules that need to run after all exploration for a (group, expression) pair is done.
ruleSet.getMatchPartitionRules(rule -> configuration.isRuleEnabled(rule))
ruleSet.getMatchPartitionRules(configuration::isRuleEnabled)
.filter(this::shouldPushRule)
.forEach(this::pushTransformMatchPartition);

// This is closely tied to the way that rule finding works _now_. Specifically, rules are indexed only
// by the type of their _root_, not any of the stuff lower down. As a result, we have enough information
// right here to determine the set of all possible rules that could ever be applied here, regardless of
// what happens towards the leaves of the tree.
ruleSet.getRules(getExpression(), rule -> configuration.isRuleEnabled(rule))
ruleSet.getRules(getExpression(), configuration::isRuleEnabled)
.filter(rule -> !(rule instanceof PreOrderRule) &&
shouldPushRule(rule))
.forEach(this::pushTransformTask);
Expand All @@ -845,17 +854,45 @@
.map(Quantifier::getRangesOver)
.forEach(this::pushExploreGroup);

ruleSet.getRules(getExpression(), rule -> configuration.isRuleEnabled(rule))
ruleSet.getRules(getExpression(), configuration::isRuleEnabled)
.filter(rule -> rule instanceof PreOrderRule &&
shouldPushRule(rule))
.forEach(this::pushTransformTask);
return true;
}

protected abstract boolean shouldPushRule(@Nonnull CascadesRule<?> rule);

/**
* Pushes a task that transforms the current group/expression pair using the given rule. For an ordinary rule,
* pushes a {@link TransformExpression} task. For a {@link ConditionalCascadesRule}, pushes a single
* {@link ConditionalTransformExpression} task holding the enabled inner rules (but only if there are any).
*/
private void pushTransformTask(@Nonnull CascadesRule<? extends RelationalExpression> rule) {
taskStack.push(new TransformExpression(getPlannerPhase(), getGroup(), getExpression(), rule));
final PlannerPhase phase = getPlannerPhase();
final Reference group = getGroup();
final RelationalExpression expression = getExpression();
if (rule instanceof final ConditionalCascadesRule<?, ?> conditionalRule) {
// Filter the individual inner rules by `configuration::isRuleEnabled` here, since the filter that is
// applied earlier at `ruleSet.getRules()` pertains only to the `ConditionalCascadesRule` wrapper.
final var enabledRules = getEnabledRules(conditionalRule);
if (!enabledRules.isEmpty()) {
taskStack.push(new ConditionalTransformExpression(phase, group, expression, enabledRules));
}
} else {
taskStack.push(new TransformExpression(phase, group, expression, rule));
}
}

/**
* Returns the inner rules of the given {@link ConditionalCascadesRule} that are currently enabled according to
* the planner {@link #configuration}, preserving their order.
*/
private <T extends RelationalExpression> List<AbstractCascadesRule<T>> getEnabledRules(ConditionalCascadesRule<?, ?> rule) {
final var rules = rule.getRules(configuration::isRuleEnabled);
@SuppressWarnings("unchecked") final var result = (List<AbstractCascadesRule<T>>)rules;
return result;
}

Check warning on line 895 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/CascadesPlanner.java

View check run for this annotation

fdb.teamscale.io / Teamscale | Test Gaps

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/CascadesPlanner.java#L891-L895 

[Test Gap] Added method `getEnabledRules` has not been tested.
https://fdb.teamscale.io/metrics/code/foundationdb-fdb-record-layer/fdb-record-layer-core%2Fsrc%2Fmain%2Fjava%2Fcom%2Fapple%2Ffoundationdb%2Frecord%2Fquery%2Fplan%2Fcascades%2FCascadesPlanner.java?coverage-mode=test-gap&t=FORK_MR%2F4332%2FRobertBrunel%2Fconditional-1%3AHEAD&selection=891-895&merge-request=FoundationDB%2Ffdb-record-layer%2F4332

private void pushTransformMatchPartition(AbstractCascadesRule<? extends MatchPartition> rule) {
taskStack.push(new TransformMatchPartition(getPlannerPhase(), getGroup(), getExpression(), rule));
Expand Down Expand Up @@ -1007,43 +1044,45 @@
*/
@Override
@SuppressWarnings("java:S1117")
public void execute() {
public boolean execute() {
if (!shouldExecute()) {
return;
return false;
}

final PlannerBindings initialBindings = getInitialBindings();
final Object bindable = getBindable();
final Iterable<PlannerBindings> boundMatches =
rule.getMatcher().bindMatches(getConfiguration(), initialBindings, bindable)::iterator;
int numMatches = 0;
boolean hasMadeProgress = false;
for (final var bindings : boundMatches) {
final var ruleCall = new CascadesRuleCall(plannerPhase, planContext, rule, group, traversal, taskStack,
bindings, evaluationContext);
++numMatches;
if (isMaxNumMatchesPerRuleCallExceeded(configuration, numMatches)) {
throw new RecordQueryPlanComplexityException("Maximum number of matches per rule call has been exceeded")
.addLogInfo(LogMessageKeys.RULE, ruleCall)
.addLogInfo(LogMessageKeys.RULE_MATCHES_COUNT, numMatches)
.addLogInfo(LogMessageKeys.MAX_RULE_MATCHES_COUNT,
configuration.getMaxNumMatchesPerRuleCall());
}
// we notify the debugger (if installed) that the transform task is succeeding and
// about begin and end of the rule call event
PlannerEventListeners.dispatchEvent(() -> toTaskEvent(Location.MATCH_PRE));
PlannerEventListeners.dispatchEvent(() -> new TransformRuleCallPlannerEvent(plannerPhase, currentRoot,
taskStack, Location.BEGIN, group, bindable, rule, ruleCall));
try {
hasMadeProgress |= executeRuleCall(ruleCall);
} finally {
PlannerEventListeners.dispatchEvent(() -> new TransformRuleCallPlannerEvent(plannerPhase, currentRoot,
taskStack, Location.END, group, bindable, rule, ruleCall));
}
}
return hasMadeProgress;
}

protected boolean executeRuleCall(@Nonnull CascadesRuleCall ruleCall) {
boolean hasMadeProgress = false;

final AtomicInteger numMatches = new AtomicInteger(0);

rule.getMatcher()
.bindMatches(getConfiguration(), initialBindings, getBindable())
.map(bindings -> new CascadesRuleCall(plannerPhase, planContext, rule, group,
traversal, taskStack, bindings, evaluationContext))
.forEach(ruleCall -> {
int ruleMatchesCount = numMatches.incrementAndGet();
if (isMaxNumMatchesPerRuleCallExceeded(configuration, ruleMatchesCount)) {
throw new RecordQueryPlanComplexityException("Maximum number of matches per rule call has been exceeded")
.addLogInfo(LogMessageKeys.RULE, ruleCall)
.addLogInfo(LogMessageKeys.RULE_MATCHES_COUNT, ruleMatchesCount)
.addLogInfo(LogMessageKeys.MAX_RULE_MATCHES_COUNT,
configuration.getMaxNumMatchesPerRuleCall());
}
// we notify the debugger (if installed) that the transform task is succeeding and
// about begin and end of the rule call event
PlannerEventListeners.dispatchEvent(() -> toTaskEvent(Location.MATCH_PRE));
PlannerEventListeners.dispatchEvent(() -> new TransformRuleCallPlannerEvent(plannerPhase, currentRoot,
taskStack, Location.BEGIN, group, getBindable(), rule, ruleCall));
try {
executeRuleCall(ruleCall);
} finally {
PlannerEventListeners.dispatchEvent(() -> new TransformRuleCallPlannerEvent(plannerPhase, currentRoot,
taskStack, Location.END, group, getBindable(), rule, ruleCall));
}
});
}

protected void executeRuleCall(@Nonnull CascadesRuleCall ruleCall) {
ruleCall.run();

//
Expand All @@ -1059,18 +1098,21 @@
PlannerEventListeners.dispatchEvent(() -> new TransformRuleCallPlannerEvent(plannerPhase, currentRoot, taskStack,
Location.YIELD, group, getBindable(), rule, ruleCall));
taskStack.push(new AdjustMatch(getPlannerPhase(), getGroup(), getExpression(), newPartialMatch));
hasMadeProgress = true;
}

for (final RelationalExpression newExpression : ruleCall.getNewFinalExpressions()) {
PlannerEventListeners.dispatchEvent(() -> new TransformRuleCallPlannerEvent(plannerPhase, currentRoot, taskStack,
Location.YIELD, group, getBindable(), rule, ruleCall));
exploreExpressionAndOptimizeInputs(plannerPhase, getGroup(), newExpression, true);
hasMadeProgress = true;
}

for (final RelationalExpression newExpression : ruleCall.getNewExploratoryExpressions()) {
PlannerEventListeners.dispatchEvent(() -> new TransformRuleCallPlannerEvent(plannerPhase, currentRoot, taskStack,
Location.YIELD, group, getBindable(), rule, ruleCall));
exploreExpression(plannerPhase, group, newExpression, true);
hasMadeProgress = true;
}

final var referencesWithPushedRequirements = ruleCall.getReferencesWithPushedConstraints();
Expand All @@ -1084,13 +1126,16 @@
//
if (!(rule instanceof PreOrderRule)) {
taskStack.push(this);
hasMadeProgress = true;
}
for (final Reference reference : referencesWithPushedRequirements) {
if (!reference.hasNeverBeenExplored()) {
taskStack.push(new ExploreGroup(plannerPhase, reference));
hasMadeProgress = true;
}
}
}
return hasMadeProgress;
}

@Override
Expand Down Expand Up @@ -1137,6 +1182,47 @@
}
}

/**
* Class to transform an expression using the first applicable rule out of an ordered list of rules, as produced by
* a {@link ConditionalCascadesRule}. The rules are tried one at a time. This task first applies the rule at
* {@code index} (by delegating to the {@link TransformExpression} superclass). If that rule makes no progress,
* it pushes a follow-up {@code ConditionalTransformExpression} that resumes at the next index.
*/
private class ConditionalTransformExpression extends TransformExpression {
@Nonnull
private final List<AbstractCascadesRule<? extends RelationalExpression>> rules;
private final int index;

public ConditionalTransformExpression(@Nonnull final PlannerPhase plannerPhase,
@Nonnull final Reference group,
@Nonnull final RelationalExpression expression,
@Nonnull final List<? extends AbstractCascadesRule<? extends RelationalExpression>> rules) {
this(plannerPhase, group, expression, ImmutableList.copyOf(rules), 0);
}

Check warning on line 1201 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/CascadesPlanner.java

View check run for this annotation

fdb.teamscale.io / Teamscale | Test Gaps

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/CascadesPlanner.java#L1196-L1201 

[Test Gap] Added method `ConditionalTransformExpression` has not been tested.
https://fdb.teamscale.io/metrics/code/foundationdb-fdb-record-layer/fdb-record-layer-core%2Fsrc%2Fmain%2Fjava%2Fcom%2Fapple%2Ffoundationdb%2Frecord%2Fquery%2Fplan%2Fcascades%2FCascadesPlanner.java?coverage-mode=test-gap&t=FORK_MR%2F4332%2FRobertBrunel%2Fconditional-1%3AHEAD&selection=1196-1201&merge-request=FoundationDB%2Ffdb-record-layer%2F4332

private ConditionalTransformExpression(@Nonnull final PlannerPhase plannerPhase,
@Nonnull final Reference group,
@Nonnull final RelationalExpression expression,
@Nonnull final List<AbstractCascadesRule<? extends RelationalExpression>> rules,
final int index) {
super(plannerPhase, group, expression, rules.get(index));
this.rules = rules;
this.index = index;
}

Check warning on line 1211 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/CascadesPlanner.java

View check run for this annotation

fdb.teamscale.io / Teamscale | Test Gaps

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/CascadesPlanner.java#L1203-L1211 

[Test Gap] Added method `ConditionalTransformExpression` has not been tested.
https://fdb.teamscale.io/metrics/code/foundationdb-fdb-record-layer/fdb-record-layer-core%2Fsrc%2Fmain%2Fjava%2Fcom%2Fapple%2Ffoundationdb%2Frecord%2Fquery%2Fplan%2Fcascades%2FCascadesPlanner.java?coverage-mode=test-gap&t=FORK_MR%2F4332%2FRobertBrunel%2Fconditional-1%3AHEAD&selection=1203-1211&merge-request=FoundationDB%2Ffdb-record-layer%2F4332

@Override
public boolean execute() {
if (super.execute()) {

@RobertBrunel RobertBrunel Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also take into account shouldExecute() here, and immediately prune all the inner rules if false?

if (!shouldExecute()) {
  return false;
}

return true;
}
if (index + 1 < rules.size()) {
taskStack.push(new ConditionalTransformExpression(getPlannerPhase(), getGroup(), getExpression(),
rules, index + 1));
}
return false;
}

Check warning on line 1223 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/CascadesPlanner.java

View check run for this annotation

fdb.teamscale.io / Teamscale | Test Gaps

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/CascadesPlanner.java#L1214-L1223 

[Test Gap] Added method `execute` has not been tested.
https://fdb.teamscale.io/metrics/code/foundationdb-fdb-record-layer/fdb-record-layer-core%2Fsrc%2Fmain%2Fjava%2Fcom%2Fapple%2Ffoundationdb%2Frecord%2Fquery%2Fplan%2Fcascades%2FCascadesPlanner.java?coverage-mode=test-gap&t=FORK_MR%2F4332%2FRobertBrunel%2Fconditional-1%3AHEAD&selection=1214-1223&merge-request=FoundationDB%2Ffdb-record-layer%2F4332
}

/**
* Class to transform a match partition using a rule.
*/
Expand Down Expand Up @@ -1205,12 +1291,13 @@
}

@Override
public void execute() {
public boolean execute() {
final var ruleSet = getPlannerPhase().getRuleSet();
ruleSet.getPartialMatchRules(rule -> configuration.isRuleEnabled(rule))
ruleSet.getPartialMatchRules(configuration::isRuleEnabled)
.forEach(rule ->
taskStack.push(new TransformPartialMatch(getPlannerPhase(), getGroup(), getExpression(),
partialMatch, rule)));
return true;
}

@Override
Expand Down Expand Up @@ -1260,14 +1347,15 @@
}

@Override
public void execute() {
public boolean execute() {
if (!group.containsExactly(expression)) {
return;
return false;
}
for (final Quantifier quantifier : expression.getQuantifiers()) {
final Reference rangesOver = quantifier.getRangesOver();
taskStack.push(new OptimizeGroup(plannerPhase, rangesOver));
}
return true;
}

@Override
Expand Down
Loading
Loading