Skip to content

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

Merged
merged 4 commits into from
Apr 25, 2025

Conversation

normen662
Copy link
Contributor

@normen662 normen662 commented Apr 23, 2025

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 to RecordQueryPlan from the innards of the planner. This PR specifically changes the PropertiesMap that uses RecordQueryPlan with a generic version that can either use RelationalExpression or RecordQueryPlan 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:

  1. properties are singletons (or multitons; enumerated singletons)
  2. properties can create a visitor using createVisitor()
  3. all properties use visitors that work on all kinds of expressions
  4. properties that are not meaningful for all kinds of expressions (plans vs all expressions) can refuse to return a proper result for those (by throwing a RecordCoreException)
  5. see SimpleExpressionVisitor and ExpressionProperty.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.
  6. previous classes called ...Property are not properties anymore if they cannot be expressed using 1. and 2. Those properties have been demoted to visitors

@normen662 normen662 requested review from alecgrieser and hatyo April 23, 2025 16:31
@normen662 normen662 added the enhancement New feature or request label Apr 24, 2025
Copy link
Collaborator

@alecgrieser alecgrieser left a 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

@normen662 normen662 removed the request for review from hatyo April 25, 2025 06:53
Copy link
Collaborator

@alecgrieser alecgrieser left a 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

@normen662 normen662 force-pushed the prepare-query-rewrite branch from 1e3d73a to 3658664 Compare April 25, 2025 15:03
@alecgrieser alecgrieser changed the title prepare planner for query rewrites Prepare the Cascades planner for multi-stage planning by enabling property computation on arbitrary expressions Apr 25, 2025
@normen662 normen662 merged commit a5c4a13 into FoundationDB:main Apr 25, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants