Skip to content
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

Add ApplyOrder::Recursive to replace None: Option<ApplyOrder> and remove nested pattern matching. #4917

Open
HaoYang670 opened this issue Jan 15, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@HaoYang670
Copy link
Contributor

HaoYang670 commented Jan 15, 2023

Currently, we use None to represent an optimizing rule to be recursively applied to its children.
And we use Some(TopDown) and Some(BottomUp) to represent a rule can be directly applied to the child node.

The None usage can be directly replaced by introducing a new product type ApplyOrder::Recursive.
This can

  • remove the usage of nested pattern matching,
  • better comment the code ,
  • and help the rust compiler to better optimize the code.

This is a follow-up of #4618.

Describe the solution you'd like

pub enum ApplyOrder {
     /// ...
    TopDown,
    /// ...
    BottomUp,
    /// ...
    Recursive,
}

Describe alternatives you've considered
We could not do this.

Additional context
Add any other context or screenshots about the feature request here.

@HaoYang670 HaoYang670 added the enhancement New feature or request label Jan 15, 2023
@HaoYang670 HaoYang670 changed the title Add a sum type ApplyOrder::Recursive to replace None: Option<ApplyOrder> and avoid nested pattern matching. Add a sum type ApplyOrder::Recursive to replace None: Option<ApplyOrder> and remove nested pattern matching. Jan 15, 2023
@HaoYang670 HaoYang670 changed the title Add a sum type ApplyOrder::Recursive to replace None: Option<ApplyOrder> and remove nested pattern matching. Add ApplyOrder::Recursive to replace None: Option<ApplyOrder> and remove nested pattern matching. Jan 15, 2023
@HaoYang670
Copy link
Contributor Author

HaoYang670 commented Jan 17, 2023

Hi @jackwener, I have one question about the optimize_node function:

    fn optimize_node(
        &self,
        rule: &Arc<dyn OptimizerRule + Send + Sync>,
        plan: &LogicalPlan,
        config: &dyn OptimizerConfig,
    ) -> Result<Option<LogicalPlan>> {
        // TODO: future feature: We can do Batch optimize
        rule.try_optimize(plan, config)
    }

Does it only optimize the top level plan node or the whole plan tree?
I don't understand the difference between it and the try_optimize.

@HaoYang670
Copy link
Contributor Author

And I am currently confused about the implementation difference between Some(ApplyOrder::TopDown) and None. Both of them call the try_optimize in the function optimize_recursively.

@jackwener
Copy link
Member

Does it only optimize the top level plan node or the whole plan tree? I don't understand the difference between it and the try_optimize.

We can do batch optimize in one plannode

 rules: Vec<rule>
for rule in rules {
    rule.try_optimize(plan, config)
}

@jackwener
Copy link
Member

And I am currently confused about the implementation difference between Some(ApplyOrder::TopDown) and None. Both of them call the try_optimize in the function optimize_recursively.

When use None, try_optimize is just entry (it just use for top plannode), the internal implementation (topdown/bottomup) is inside rule.

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

No branches or pull requests

2 participants