Skip to content

Leverage rule indexing for CollapseNullStrictValueOverNullValueRule #4260

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
+43 −34
Open

Leverage rule indexing for CollapseNullStrictValueOverNullValueRule #4260

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
52 changes: 20 additions & 32 deletions ...d/query/plan/cascades/values/simplification/CollapseNullStrictValueOverNullValueRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,64 +23,52 @@
import com.apple.foundationdb.annotation.API;
import com.apple.foundationdb.record.query.plan.cascades.matching.structure.BindingMatcher;
import com.apple.foundationdb.record.query.plan.cascades.values.ArithmeticValue;
import com.apple.foundationdb.record.query.plan.cascades.values.CastValue;
import com.apple.foundationdb.record.query.plan.cascades.values.FieldValue;
import com.apple.foundationdb.record.query.plan.cascades.values.NotValue;
import com.apple.foundationdb.record.query.plan.cascades.values.NullValue;
import com.apple.foundationdb.record.query.plan.cascades.values.PromoteValue;
import com.apple.foundationdb.record.query.plan.cascades.values.SubscriptValue;
import com.apple.foundationdb.record.query.plan.cascades.values.Value;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Streams;

import javax.annotation.Nonnull;
import java.util.Optional;

import static com.apple.foundationdb.record.query.plan.cascades.matching.structure.TypedMatcherWithPredicate.typedMatcherWithPredicate;

/**
* A rule that collapses a {@link Value} into a {@link NullValue} if it is strictly null-propagating and has at least
* one child that is a {@link NullValue}. A {@code Value} class is <em>strictly null-propagating</em> if it produces a
* null result when any child evaluates to null. For example, arithmetic operators ({@link ArithmeticValue}) yield null

Check notice on line 36 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/simplification/CollapseNullStrictValueOverNullValueRule.java

View workflow job for this annotation

GitHub Actions / coverage 

File coverage: 100.0% (13/13 lines) | Changed lines: 100.0% (9/9 lines)
* when either operand is null. For the particular classes that are considered by this rule, see {@link #VALUE_CLASSES}.
* when either operand is null.
*
* <p>The rule is parameterized over the concrete, strictly null-propagating {@link Value} subclass. This way it can use
* a typed matcher for the specific class and benefit from the rule index in the rule set.
*
* @param <V> the {@link Value} subclass that this rule instance matches
*/
@API(API.Status.EXPERIMENTAL)
public class CollapseNullStrictValueOverNullValueRule extends ValueSimplificationRule<Value> {

/**
* {@link Value} subclasses that are considered to be strictly null-propagating by this rule.
*/
@Nonnull
private static final ImmutableSet<Class<? extends Value>> VALUE_CLASSES = ImmutableSet.of(
ArithmeticValue.class,
CastValue.class,
FieldValue.class,
NotValue.class,
PromoteValue.class,
SubscriptValue.class);
public final class CollapseNullStrictValueOverNullValueRule<V extends Value> extends ValueSimplificationRule<V> {

@Nonnull
private static final BindingMatcher<Value> rootMatcher = typedMatcherWithPredicate(Value.class,
v -> VALUE_CLASSES.contains(v.getClass()) && hasNullValueChild(v));
private final BindingMatcher<V> rootMatcher;

public CollapseNullStrictValueOverNullValueRule() {
super(rootMatcher);
public CollapseNullStrictValueOverNullValueRule(@Nonnull final Class<V> valueClass) {
this(typedMatcherWithPredicate(valueClass, CollapseNullStrictValueOverNullValueRule::hasNullValueChild));
}

@Nonnull
@Override
public Optional<Class<?>> getRootOperator() {
return Optional.empty();
private CollapseNullStrictValueOverNullValueRule(@Nonnull final BindingMatcher<V> rootMatcher) {
super(rootMatcher);
this.rootMatcher = rootMatcher;
}

@Override
public void onMatch(@Nonnull final ValueSimplificationRuleCall call) {
final Value value = call.getBindings().get(rootMatcher);
final V value = call.getBindings().get(rootMatcher);
// Note that the `NullValue` will always have a nullable result type even if the value’s result type is not.
call.yieldResult(new NullValue(value.getResultType()));
}

private static boolean hasNullValueChild(@Nonnull final Value value) {
return Streams.stream(value.getChildren()).anyMatch(NullValue.class::isInstance);
for (final Value child : value.getChildren()) {
if (child instanceof NullValue) {
return true;
}
}
return false;
}
}

Check warning on line 74 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/simplification/CollapseNullStrictValueOverNullValueRule.java

View check run for this annotation

fdb.teamscale.io / Teamscale | Findings

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/simplification/CollapseNullStrictValueOverNullValueRule.java#L45-L74 

[New] `CollapseNullStrictValueOverNullValueRule` has inheritance depth of 4 which is deeper than maximum of 2
https://fdb.teamscale.io/findings/details/foundationdb-fdb-record-layer?id=066F1E6C3177E771DA0D3F30F854070D&t=FORK_MR%2F4260%2FRobertBrunel%2FCollapseNullStrictValueOverNullValueRule%3AHEAD
25 changes: 23 additions & 2 deletions ...b/record/query/plan/cascades/values/simplification/DefaultValueSimplificationRuleSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,13 @@

package com.apple.foundationdb.record.query.plan.cascades.values.simplification;

import com.apple.foundationdb.annotation.API;

Check notice on line 23 in fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/simplification/DefaultValueSimplificationRuleSet.java

View workflow job for this annotation

GitHub Actions / coverage 

File coverage: 100.0% (15/15 lines) | Changed lines: 100.0% (6/6 lines)
import com.apple.foundationdb.record.query.plan.cascades.values.ArithmeticValue;
import com.apple.foundationdb.record.query.plan.cascades.values.CastValue;
import com.apple.foundationdb.record.query.plan.cascades.values.FieldValue;
import com.apple.foundationdb.record.query.plan.cascades.values.NotValue;
import com.apple.foundationdb.record.query.plan.cascades.values.PromoteValue;
import com.apple.foundationdb.record.query.plan.cascades.values.SubscriptValue;
import com.apple.foundationdb.record.query.plan.cascades.values.Value;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSetMultimap;
Expand All @@ -36,7 +42,17 @@
@SuppressWarnings("java:S1452")
public class DefaultValueSimplificationRuleSet extends AbstractValueRuleSet<Value, ValueSimplificationRuleCall> {
@Nonnull
protected static final ValueSimplificationRule<? extends Value> collapseNullStrictValueOverNullValueRule = new CollapseNullStrictValueOverNullValueRule();
protected static final ValueSimplificationRule<? extends Value> collapseNullStrictArithmeticValueOverNullValueRule = new CollapseNullStrictValueOverNullValueRule<>(ArithmeticValue.class);
@Nonnull
protected static final ValueSimplificationRule<? extends Value> collapseNullStrictCastValueOverNullValueRule = new CollapseNullStrictValueOverNullValueRule<>(CastValue.class);
@Nonnull
protected static final ValueSimplificationRule<? extends Value> collapseNullStrictFieldValueOverNullValueRule = new CollapseNullStrictValueOverNullValueRule<>(FieldValue.class);
@Nonnull
protected static final ValueSimplificationRule<? extends Value> collapseNullStrictNotValueOverNullValueRule = new CollapseNullStrictValueOverNullValueRule<>(NotValue.class);
@Nonnull
protected static final ValueSimplificationRule<? extends Value> collapseNullStrictPromoteValueOverNullValueRule = new CollapseNullStrictValueOverNullValueRule<>(PromoteValue.class);
@Nonnull
protected static final ValueSimplificationRule<? extends Value> collapseNullStrictSubscriptValueOverNullValueRule = new CollapseNullStrictValueOverNullValueRule<>(SubscriptValue.class);
@Nonnull
protected static final ValueSimplificationRule<? extends Value> composeFieldValueOverRecordConstructorRule = new ComposeFieldValueOverRecordConstructorRule();
@Nonnull
Expand All @@ -48,7 +64,12 @@
// See Simplification.executeRuleSetIteratively for more details.
@Nonnull
protected static final Set<ValueSimplificationRule<? extends Value>> SIMPLIFICATION_RULES = ImmutableSet.of(
collapseNullStrictValueOverNullValueRule,
collapseNullStrictArithmeticValueOverNullValueRule,
collapseNullStrictCastValueOverNullValueRule,
collapseNullStrictFieldValueOverNullValueRule,
collapseNullStrictNotValueOverNullValueRule,
collapseNullStrictPromoteValueOverNullValueRule,
collapseNullStrictSubscriptValueOverNullValueRule,
composeFieldValueOverRecordConstructorRule,
composeFieldValueOverFieldValueRule,
collapseRecordConstructorOverFieldsToStarRule);
Expand Down
Loading