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

[EPIC]: reimplement all rules which contains global-state #4267

Closed
6 tasks done
jackwener opened this issue Nov 18, 2022 · 7 comments · Fixed by #4465
Closed
6 tasks done

[EPIC]: reimplement all rules which contains global-state #4267

jackwener opened this issue Nov 18, 2022 · 7 comments · Fixed by #4465
Labels
enhancement New feature or request

Comments

@jackwener
Copy link
Member

jackwener commented Nov 18, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

Current some rule contains global-state

Such as limit_push_down use ancestor: &Ancestor collect the limit-state of whole tree.

Describe the solution you'd like
A clear and concise description of what you want to happen.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

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

@jackwener jackwener added the enhancement New feature or request label Nov 18, 2022
@jackwener jackwener changed the title [EPIC]: reimplement all rule which contains global-state [EPIC]: reimplement all rules which contains global-state Nov 18, 2022
@jackwener
Copy link
Member Author

jackwener commented Nov 18, 2022

let me explain why.

Due to global-state exists, we can't just optimize a subtree, we must traverse the whole tree.

In fact, I prefer to Pattern-match, we match a subtree pattern, and just optimize the pattren/subtree.

A example: Merge Limit

match plan {
     Limit =>  {
           match childPlan => {
              Limit => {  merge(limit, childlimit)  }
               _ => { don't match pattern, return None or recurse....}
           }
     }
     _ => { don't match pattern, return None or recurse....}
}

we just match limit-limit, and handle it.

BUT, global-state force we must handle various situations, and we must traverse the tree all.

What's more, global-stats is easy to cause bugs. Such as projection exist alias, the global-stats exist bug.

@jackwener
Copy link
Member Author

After finish this job,
I will need to finish #4208 .
After finish them, All preparations will be done.
I will do #3972 ! It will make our optimizer simple and easy-use.

@jackwener
Copy link
Member Author

@alamb
Copy link
Contributor

alamb commented Nov 18, 2022

👍 the idea of pattern matching on the way back up during rewrite rules makes lots of sense to me 👍

@mingmwang
Copy link
Contributor

I think it depends on what the global-state is. If the global states are predicates or something related to Column/Exprs, then we should remove such kind of global states.. But for some optimization rules, it might be better to keep a global state so that it is easily to figure out a best plan.

@mingmwang
Copy link
Contributor

mingmwang commented Nov 28, 2022

I think today in DataFusion's logical optimizer rules, most of the complexity comes from dealing with predicates. That's why I want to introduce the QueryConstraints into the logical plan structs.

#4222

@jackwener
Copy link
Member Author

jackwener commented Nov 28, 2022

I think it depends on what the global-state is. If the global states are predicates or something related to Column/Exprs, then we should remove such kind of global states.. But for some optimization rules, it might be better to keep a global state so that it is easily to figure out a best plan.

Agree it, global-state of all rules that I change is related Column/Exprs or limit number.

I think today in DataFusion's logical optimizer rules, most of the complexity comes from dealing with predicates.

Agree, especially for rule about projection/filter/agg.

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 a pull request may close this issue.

3 participants