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

feat(planner): Introduce bitmap to record applied rules #10024

Merged
merged 45 commits into from
Mar 1, 2023

Conversation

dusx1981
Copy link
Contributor

@dusx1981 dusx1981 commented Feb 13, 2023

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

Summary

A draft pr for code review.

closes #9536

1: Use RuleFactor class classification to manage rule sets
2: Add rule classification to rule
In order to compare the test results later, the old code is retained.
Add MetadataRef for CascadesOptimizer and Memo
@vercel
Copy link

vercel bot commented Feb 13, 2023

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

1 Ignored Deployment
Name Status Preview Comments Updated
databend ⬜️ Ignored (Inspect) Visit Preview Feb 27, 2023 at 11:20PM (UTC)

@dusx1981 dusx1981 marked this pull request as ready for review February 13, 2023 13:03
@mergify
Copy link
Contributor

mergify bot commented Feb 13, 2023

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

The description should contain the following:

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

## Summary

Summary about this PR

Close #issue

@mergify
Copy link
Contributor

mergify bot commented Feb 13, 2023

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 dusx1981 marked this pull request as draft February 14, 2023 00:56
@dusx1981 dusx1981 requested review from Xuanwo and removed request for leiysky February 25, 2023 08:03
@dusx1981 dusx1981 marked this pull request as ready for review February 25, 2023 08:04
@dusx1981
Copy link
Contributor Author

The problem with rule application has been solved and the code can already be reviewed.

@dusx1981
Copy link
Contributor Author

dusx1981 commented Feb 27, 2023

Solve the problem:

  1. (x..y) is a half-open and half-closed interval, my initial implementation misses the last rule
  2. The optimization result of HeuristicOptimizer is related to the order of the application rules, and the adjustment of the application rules is consistent with original order.

Copy link
Collaborator

@leiysky leiysky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

@dusx1981
Copy link
Contributor Author

image

This issue caused by you didn't use our PR description template, and now it has been fixed.

looking forward to your review :).

@xudong963
Copy link
Member

Just notice the ticket, let me take a look before merging...

@BohuTANG
Copy link
Member

Hi @dusx1981 ,

Nice, thanks for your contribution!
Can you give more summary for this PR? It would be great to add some logic tests for it if possible(Like

explain select * from (select t.number from numbers(10) as t limit 8) limit 9
).

@xudong963
Copy link
Member

It would be great to add some logic tests for it if possible

I think the current tests are covered. The ticket only changes the way to record optimization rules.

@dusx1981 dusx1981 requested review from xudong963 and removed request for Xuanwo February 27, 2023 09:48
@dusx1981
Copy link
Contributor Author

Do I need to change the status to resolved after answering the question?

@dusx1981
Copy link
Contributor Author

Metadata information in CascadesOptimizer and Memo has been removed:)

@sundy-li sundy-li merged commit a270653 into datafuselabs:main Mar 1, 2023
@sundy-li
Copy link
Member

sundy-li commented Mar 1, 2023

LGTM, thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Introduce bitmap to record applied rules
6 participants