fix(planner): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES #10508

dusx1981 · 2023-03-12T01:22:57Z

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

The new map
The new mapping relationship between RuleID and DEFAULT_REWRITE_RULES looks like this：

RuleIDs are put together according to the type，so each operator can apply its own rules in the correct order.

The scalability
Considering that each operator only applies its own rules in the end, the new mapping only reduces the number of loops compared to the old implementation, and does not affect the addition of new rules.

Optimization of the heuristic optimizer

Filter: Reduce the number of loops from 22 to 10

EvalScalar: Reduce the number of loops from 22 to 2

Limit: Reduce the number of loops from 22 to 6

Agg: Reduce the number of loops from 22 to 2

Sort: Reduce the number of loops from 22 to 1

Scan: Reduce the number of loops from 22 to 0

DumpTableScan: Reduce the number of loops from 22 to 0

The sample code
some operators are not required to apply the rules:

impl Operator for Scan {

    // Returns the set of rules that the operator can apply
    fn transformation_candidate_rules(&self) -> roaring::RoaringBitmap {
        RoaringBitmap::new()
    }

impl Operator for DummyTableScan {

    fn transformation_candidate_rules(&self) -> roaring::RoaringBitmap {
        RoaringBitmap::new()
    }

Some operators require only a few rules to be applied:

    // EvalScalar
    EliminateEvalScalar,
    MergeEvalScalar,

    // Sort
    PushDownSortScan,

    // EvalScalar
    EliminateEvalScalar,
    MergeEvalScalar,

And with the continuous increase of rules, it may eventually reach hundreds. With recursive calls, the magnitude of the loop may grow exponentially.So I think it's necessary to just apply it's own rules for each operator.

In this way, the number of loops per optimization can be greatly reduced.

Closes #issue

This reverts commit 22b26ca.

…ULES

vercel · 2023-03-12T01:23:01Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
databend	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Mar 19, 2023 at 7:44PM (UTC)

dusx1981 · 2023-03-12T22:11:57Z

@leiysky

Because of the precise management of the application of operator rules，please do a benchmark test:)

one of the implementations：

impl Operator for Limit {
    fn transformation_candidate_rules(&self) -> roaring::RoaringBitmap {
        (RuleID::PushDownLimitUnion as u32..(RuleID::PushDownLimitScan as u32) + 1)
            .collect::<RoaringBitmap>()
    }

AND I'm collating the preprocessing logic that needs to be done from GPORCA

mergify · 2023-03-13T06:50:40Z

This pull request's title is not fulfill the requirements. @dusx1981 please update it 🙏.

Valid format:

fix(query): fix group by string bug
  ^         ^---------------------^
  |         |
  |         +-> Summary in present tense.
  |
  +-------> Type: rfc, feat, fix, refactor, ci, docs, chore

Valid types:

rfc: this PR proposes a new RFC
feat: this PR introduces a new feature to the codebase
fix: this PR patches a bug in codebase
refactor: this PR changes the code base without new features or bugfix
ci: this PR changes build/testing/ci steps
docs: this PR changes the documents or websites
chore: this PR only has small changes that no need to record

dusx1981 · 2023-03-13T11:26:50Z

@BohuTANG

Could you please run a benchmark test to see if this PR should be reviewed?

xudong963 · 2023-03-19T09:11:55Z

@mergify update

mergify · 2023-03-19T09:11:58Z

update

❌ Pull request can't be updated with latest base branch changes

Mergify needs the author permission to update the base branch of the pull request.
@dusx1981 needs to authorize modification on its head branch.
err-code: ED112

dusx1981 · 2023-03-19T09:23:36Z

@mergify update

mergify · 2023-03-19T09:23:39Z

update

☑️ Nothing to do

#commits-behind>0 [:pushpin: update requirement]
-closed [:pushpin: update requirement]

dusx1981 · 2023-03-19T09:25:49Z

@xudong963

@mergify update

dusx1981 · 2023-03-19T09:49:06Z

github-actions · 2023-03-19T22:16:42Z

ClickBench Report

xudong963

Changes make sense to me. Sorry for late review, thanks @dusx1981

Any thoughts? @leiysky

leiysky · 2023-03-21T06:14:09Z

src/query/sql/src/planner/optimizer/cascades/cascade.rs

-    pub(crate) memo: Memo,
-    pub(crate) cost_model: Box<dyn CostModel>,
+    pub memo: Memo,
+    pub explore_rule_set: roaring::RoaringBitmap,


I think we'd better not expose the RoaringBitmap here, should use RuleSet instead.

I think we'd better not expose the RoaringBitmap here, should use RuleSet instead.

I understand what you mean, I want to transform the RuleSet to use bitmap management, and then submit a PR. If there is no problem with this PR, can it be passed first？

If there is no problem with this PR, can it be passed first？

Please address it in this PR.

leiysky · 2023-03-21T06:17:20Z

src/query/sql/src/planner/optimizer/heuristic/heuristic.rs

    /// Try to apply the rules to the expression.
    /// Return the final result that no rule can be applied.
    fn apply_transform_rules(&self, s_expr: &SExpr) -> Result<SExpr> {
        let mut s_expr = s_expr.clone();
+        let rule_set = self.calc_operator_rule_set(&s_expr.plan);


We should distinguish between RuleSet and RuleList. RuleSet is order-insensitive, and RuleList is order-sensitive.

In this case, a RuleList is required.

We should distinguish between RuleSet and RuleList. RuleSet is order-insensitive, and RuleList is order-sensitive.

In this case, a RuleList is required.

Please refer to the introduction of my implementation ideas above. If you use RuleList, you cannot quickly calculate the applicable rules for each operator, which will cause too many loops and reduce performance.

And Please refer to the benchmark results

@leiysky

Any other question？

And Please refer to the benchmark results

Could you please provide more details about the benchmark? The benefits appear to be relatively low.

hits: from 1.01 to 0.99

tpch: from 1.02 to 0.98

Although performance is important, I prefer to prioritize code readability and maintainability. It's not worth sacrificing those aspects for the sake of minor performance improvements.

I disagree with this point of view. With the increase of rules, each rule will increase the number of cycles optimized by unrelated operators, and it is exponential. When I think of it, the performance comparison will be more obvious.

With the increase of rules, each rule will increase the number of cycles optimized by unrelated operators, and it is exponential.

You are correct, but for now, it is not worth it. We can optimize when we identify the bottleneck.

leiysky · 2023-03-21T13:09:19Z

src/query/sql/src/planner/optimizer/cascades/tasks/explore_expr.rs

+        optimizer: &CascadesOptimizer,
+        operator: &RelOperator,
+    ) -> roaring::RoaringBitmap {
+        unsafe {


Why do we need unsafe here?

// read only, so thread safe pub static mut RULE_FACTORY: Lazy<RuleFactory> = Lazy::new(RuleFactory::create);

RULE_FACTORY is read-only, so it is thread-safe. Here, we need to use unsafe to solve the problem of syntax errors.

RULE_FACTORY is read-only, so it is thread-safe.

It's not a good reason to use unsafe.

Here, we need to use unsafe to solve the problem of syntax errors.

This reasoning is flawed. Implementing unsafe practices everywhere will ultimately lead to increased long-term maintenance costs.

leiysky · 2023-03-21T13:11:59Z

src/query/sql/src/planner/optimizer/cascades/tasks/explore_expr.rs


        for rule_id in rule_set.iter() {
            let apply_rule_task = ApplyRuleTask::with_parent(
-                rule_id,
+                unsafe { std::mem::transmute::<u8, RuleID>(rule_id as u8) },


Transmuting an enum to u8 is dangerous. Why do we need this here?

Transmuting an enum to u8 is dangerous. Why do we need this here?

Ok, first of all, the current RuleID is safe under this transformation. could you all me to handle it with standard casts in next PR?

Doesn't make sense to use mem::transmute here. Also, it should be addressed in this PR.

leiysky · 2023-03-21T13:12:27Z

src/query/sql/src/planner/optimizer/heuristic/heuristic.rs

@@ -113,13 +121,27 @@ impl HeuristicOptimizer {
        Ok(result)
    }

+    fn calc_operator_rule_set(&self, operator: &RelOperator) -> roaring::RoaringBitmap {
+        unsafe { operator.transformation_candidate_rules() & (&RULE_FACTORY.transformation_rules) }


Ditto, why do we need unsafe here?

// read only, so thread safe pub static mut RULE_FACTORY: Lazy<RuleFactory> = Lazy::new(RuleFactory::create);

RULE_FACTORY is read-only, so it is thread-safe. Here, we need to use unsafe to solve the problem of syntax errors.

RULE_FACTORY is read-only, so it is thread-safe. Here, we need to use unsafe to solve the problem of syntax errors.

Doesn't make sense to me.

dusx1981 · 2023-03-21T13:42:14Z

@leiysky

Hasn’t the same code been reviewed by you in this PR?

leiysky · 2023-03-21T14:25:31Z

@leiysky

Hasn’t the same code been reviewed by you in this PR?

I appreciate your effort, but I must admit that I couldn't accept that pull request due to its low quality.

To be frank, I don't have the energy to point out every issue with your code, and it's unnecessary.

Despite its shortcomings, I recognize the value of your work and have accepted it. However, it has caused a significant impact on the project's maintenance, and as a result, I had to refactor it.

This pull request is merely a minor patch on the previous one and even corrupts the latest codebase.

Although there is always room for improvement, I don't think this is the right way.

dusx1981 · 2023-03-21T19:33:32Z

@leiysky
Hasn’t the same code been reviewed by you in this PR?

I appreciate your effort, but I must admit that I couldn't accept that pull request due to its low quality.

To be frank, I don't have the energy to point out every issue with your code, and it's unnecessary.

Despite its shortcomings, I recognize the value of your work and have accepted it. However, it has caused a significant impact on the project's maintenance, and as a result, I had to refactor it.

This pull request is merely a minor patch on the previous one and even corrupts the latest codebase.

Although there is always room for improvement, I don't think this is the right way.

So I feel a little weird about a few points：

You completely negate your original evaluation, there is no logic at all
You completely ignore my implementation ideas above
AND in this pr, I see what you call professionalism:
You completely negate what other people have said about this project
You completely ignore the benchmark result

Xuanwo · 2023-03-22T00:36:28Z

src/query/sql/src/planner/optimizer/cascades/cascade.rs

-    pub(crate) explore_rule_set: RuleSet,
-    pub(crate) metadata: MetadataRef,
+    pub best_cost_map: HashMap<IndexType, CostContext>,
+    _ctx: Arc<dyn TableContext>,


Introducing TableContext in CascadesOptimizer may not be a good idea. Additionally, I noticed that this context has been named as _ctx. If we do not intend to use it, perhaps it would be better to remove it?

Xuanwo · 2023-03-22T00:37:05Z

src/query/sql/src/planner/optimizer/cascades/explore_rules.rs


-pub fn get_explore_rule_set(enable_bushy_join: bool) -> RuleSet {
+pub fn calc_explore_rule_set(enable_bushy_join: bool) -> RoaringBitmap {


Please refrain from breaking the current abstraction.

Xuanwo · 2023-03-22T00:41:09Z

src/query/sql/src/planner/optimizer/rule/factory.rs

+        RuleFactory {
+            transformation_rules: (RuleID::NormalizeScalarFilter as u32
+                ..RuleID::CommuteJoin as u32)
+                .collect::<RoaringBitmap>(),
+            exploration_rules: (RuleID::CommuteJoin as u32..(RuleID::RightExchangeJoin as u32) + 1)
+                .collect::<RoaringBitmap>(),
+        }
+    }


I don't understand that code. Could you explain it further? Is it safe to use as u32 for collecting? When do we need to update them? After this change, the order RuleID is sensitive?

dusx1981 · 2023-03-22T01:02:44Z

@Xuanwo

Thank you for your feedback, I also roughly know the workflow of this project, and hope to submit more qualified codes later.
I think there is no need to discuss this PR anymore, I will close it later.

Xuanwo · 2023-03-22T01:05:21Z

Thank you for your feedback, I also roughly know the workflow of this project, and hope to submit more qualified codes later.
I think there is no need to discuss this PR anymore, I will close it later.

Thanks for your effort!

dusx1981 · 2023-03-22T01:10:00Z

@Xuanwo

On the point of performance, I disagree.. With the increase of rules, each rule will increase the number of cycles optimized by unrelated operators, and it is exponential. When I think of it, the performance comparison will be more obvious.

For fast optimization, the Cascacdes optimizer has designed a way of bitmap management rules, which can quickly calculate the rules required by each operator and avoid invalid loops. Coupled with the recursive factor, the number of cycles increases exponentially. The main purpose of my implementation of these codes is to achieve this optimization.

Xuanwo · 2023-03-22T01:16:26Z

The main purpose of my implementation of these codes is to achieve this optimization.

Thanks for your effort again. We acknowledge that there is still room for improvement in Databend. However, based on the performance reports, this particular aspect is not currently a bottleneck. Therefore, we will prioritize maintaining readability and ease of use.

dusx1981 · 2023-03-22T01:18:27Z

The main purpose of my implementation of these codes is to achieve this optimization.

Thanks for your effort again. We acknowledge that there is still room for improvement in Databend. However, based on the performance reports, this particular aspect is not currently a bottleneck. Therefore, we will prioritize maintaining readability and ease of use.

Understood，Thank you.

dusx1981 · 2023-03-22T03:24:02Z

Please allow me to add an important information, I learned some key information about real database development from this Review, thank you.

dusx1981 added 2 commits March 12, 2023 08:58

Revert "refine code"

1f95502

This reverts commit 22b26ca.

Correct the mapping relationship between RuleID and DEFAULT_REWRITE_R…

376df9e

…ULES

dusx1981 marked this pull request as ready for review March 12, 2023 01:23

mergify bot added the pr-feature this PR introduces a new feature to the codebase label Mar 12, 2023

dusx1981 marked this pull request as draft March 12, 2023 01:23

fix lint

14fa243

dusx1981 marked this pull request as ready for review March 12, 2023 02:11

Merge branch 'main' into main

1e616ab

dusx1981 marked this pull request as draft March 12, 2023 02:25

dusx1981 marked this pull request as ready for review March 12, 2023 02:41

dusx1981 added 5 commits March 13, 2023 05:13

Try d DEFAULT_REWRITE_RULES

3a2dde9

Merge branch 'main' of github.com:dusx1981/databend into main

8ccbec5

try d DEFAULT_REWRITE_RULES

32d8085

reuse DEFAULT_REWRITE_RULES

1635945

adjust ruleid

ce03435

dusx1981 changed the title ~~feat(planner): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES~~ feat(planner ci): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES Mar 13, 2023

dusx1981 changed the title ~~feat(planner ci): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES~~ feat(planner): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES Mar 13, 2023

dusx1981 changed the title ~~feat(planner): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES~~ fix(planner): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES Mar 13, 2023

mergify bot added the pr-bugfix this PR patches a bug in codebase label Mar 13, 2023

dusx1981 mentioned this pull request Mar 13, 2023

chore(planner): Refine code #10488

Merged

dusx1981 changed the title ~~fix(planner): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES~~ bench(planner): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES Mar 13, 2023

dusx1981 changed the title ~~bench(planner): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES~~ ci(planner): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES Mar 13, 2023

mergify bot added the pr-build this PR changes build/testing/ci steps label Mar 13, 2023

dusx1981 changed the title ~~ci(planner): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES~~ fix(planner): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES Mar 13, 2023

dusx1981 changed the title ~~fix(planner): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES~~ ci(bench): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES Mar 13, 2023

dusx1981 changed the title ~~ci(bench): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES~~ fix(planner): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES Mar 13, 2023

xudong963 added the ci-benchmark Benchmark: run all test label Mar 19, 2023

Merge branch 'main' into main

53ee10e

vercel bot deployed to Preview March 19, 2023 09:21 View deployment

Merge branch 'datafuselabs:main' into main

e78643d

vercel bot deployed to Preview March 19, 2023 19:44 View deployment

sundy-li removed the ci-benchmark Benchmark: run all test label Mar 20, 2023

xudong963 approved these changes Mar 20, 2023

View reviewed changes

leiysky reviewed Mar 21, 2023

View reviewed changes

Xuanwo reviewed Mar 22, 2023

View reviewed changes

dusx1981 closed this Mar 22, 2023


		pub fn get_explore_rule_set(enable_bushy_join: bool) -> RuleSet {
		pub fn calc_explore_rule_set(enable_bushy_join: bool) -> RoaringBitmap {

fix(planner): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES #10508

fix(planner): Correct the mapping relationship between RuleID and DEFAULT_REWRITE_RULES #10508

Conversation

dusx1981 commented Mar 12, 2023 • edited Loading

Summary

vercel bot commented Mar 12, 2023 • edited Loading

dusx1981 commented Mar 12, 2023 • edited Loading

mergify bot commented Mar 13, 2023

dusx1981 commented Mar 13, 2023 • edited Loading

xudong963 commented Mar 19, 2023

mergify bot commented Mar 19, 2023

❌ Pull request can't be updated with latest base branch changes

dusx1981 commented Mar 19, 2023

mergify bot commented Mar 19, 2023

☑️ Nothing to do

dusx1981 commented Mar 19, 2023 • edited Loading

dusx1981 commented Mar 19, 2023

github-actions bot commented Mar 19, 2023

ClickBench Report

xudong963 left a comment

Choose a reason for hiding this comment

leiysky Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dusx1981 Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dusx1981 commented Mar 21, 2023

leiysky commented Mar 21, 2023

dusx1981 commented Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dusx1981 commented Mar 22, 2023 • edited Loading

Xuanwo commented Mar 22, 2023

dusx1981 commented Mar 22, 2023 • edited Loading

Xuanwo commented Mar 22, 2023

dusx1981 commented Mar 22, 2023

dusx1981 commented Mar 22, 2023

dusx1981 commented Mar 12, 2023 •

edited

Loading

vercel bot commented Mar 12, 2023 •

edited

Loading

dusx1981 commented Mar 12, 2023 •

edited

Loading

dusx1981 commented Mar 13, 2023 •

edited

Loading

dusx1981 commented Mar 19, 2023 •

edited

Loading

leiysky Mar 21, 2023 •

edited

Loading

dusx1981 Mar 21, 2023 •

edited

Loading

dusx1981 commented Mar 21, 2023 •

edited

Loading

dusx1981 commented Mar 22, 2023 •

edited

Loading

dusx1981 commented Mar 22, 2023 •

edited

Loading