-
Notifications
You must be signed in to change notification settings - Fork 110
Prepare the Cascades planner for multi-stage planning by enabling property computation on arbitrary expressions #3321
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things. I was also looking at the coverage information (here: https://github.com/FoundationDB/fdb-record-layer/actions/runs/14622927029/attempts/1#summary-41029106449 I also needed to download the coverage-report
for more details). There are certainly a few coverage gaps highlighted here, mainly in some of the rule implementations, which isn't amazing given the nature of the refactoring, but maybe it's something we can live with, as long as your pretty confident that the methods were just moved. Some of the things highlighted are just that the new evaluate
method taking a Reference
aren't called, which...is probably fine, I guess. For the plans, the main thing that coverage is highlighting is that some of the rules are never called, but I'm not particularly concerned with the rule changes per se, as we can be pretty confident they are correct as long as the underlying property changes are correct
...src/main/java/com/apple/foundationdb/record/query/plan/cascades/SimpleExpressionVisitor.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/apple/foundationdb/record/query/plan/cascades/SimpleExpressionVisitor.java
Show resolved
Hide resolved
...core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/ExpressionProperty.java
Show resolved
Hide resolved
...-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/PlanPropertiesMap.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/apple/foundationdb/record/query/plan/cascades/ExpressionPropertiesMap.java
Show resolved
Hide resolved
...n/java/com/apple/foundationdb/record/query/plan/cascades/properties/ExplainPlanProperty.java
Outdated
Show resolved
Hide resolved
.../foundationdb/record/query/plan/cascades/properties/NormalizedResidualPredicateProperty.java
Show resolved
Hide resolved
...n/java/com/apple/foundationdb/record/query/plan/cascades/properties/RecordTypesProperty.java
Outdated
Show resolved
Hide resolved
...n/java/com/apple/foundationdb/record/query/plan/cascades/properties/RecordTypesProperty.java
Show resolved
Hide resolved
...in/java/com/apple/foundationdb/record/query/plan/cascades/rules/ImplementTypeFilterRule.java
Show resolved
Hide resolved
...n/java/com/apple/foundationdb/record/query/plan/cascades/properties/ExplainPlanProperty.java
Outdated
Show resolved
Hide resolved
.../foundationdb/record/query/plan/cascades/properties/NormalizedResidualPredicateProperty.java
Show resolved
Hide resolved
...in/java/com/apple/foundationdb/record/query/plan/cascades/rules/ImplementTypeFilterRule.java
Show resolved
Hide resolved
...n/java/com/apple/foundationdb/record/query/plan/cascades/properties/RecordTypesProperty.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. There were two remaining typos in the Javadoc, I think, but overall, this looks good
...core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/ExpressionProperty.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/ExpressionProperty.java
Show resolved
Hide resolved
1e3d73a
to
3658664
Compare
This PR prepares the Cascades Planner for multiple planner stages. As only the last such stage produces
RecordQueryPlan
, we need to remove every explicit reference toRecordQueryPlan
from the innards of the planner. This PR specifically changes thePropertiesMap
that usesRecordQueryPlan
with a generic version that can either useRelationalExpression
orRecordQueryPlan
based on which planner stage we might be in.The second big change concerns the way properties are maintained and how state management works for visitors that are used by properties. Currently, there are properties which can be computed on plans only and others which can be computed on any kind of expression. There were also differences in how properties managed their state between different properties. The following changes were made:
createVisitor()
RecordCoreException
)SimpleExpressionVisitor
andExpressionProperty.toExpressionVisitor(..)
for details on how to transform a plan visitor to a generic expression visitor and how to transform an old-style simplistic expression visitor....Property
are not properties anymore if they cannot be expressed using 1. and 2. Those properties have been demoted to visitors