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

expression: a constraint propagate framework mainly for partition pruning #7643

Merged
merged 9 commits into from
Nov 28, 2018

Conversation

tiancaiamao
Copy link
Contributor

@tiancaiamao tiancaiamao commented Sep 7, 2018

What problem does this PR solve?

For partition pruning, we will have the partition expression such as to_days(x),
we have the constant range like:

p0 < date '2018-03-05' 
p1 < date '2018-03-01'
......

When the user send a query, such as ... where x > 632

The final partition pruning conditions could be:

p0:  to_days(x) < date '2018-03-05'   && x > 632
p1:  to_days(x) < date  '2018-03-01'  && x > 632 
...

If some conditions eval to constant false, that partition should be prune.

So, in-equal conditions and some simple function should be handled in the partition pruning process.

"a > 3" && "a > 5" => "a > 5"
"a > 5" && "a < 3" => false
to_days(t) > date '2018-03-05' && to_days(t) < '2018-03-01' => false

Ref #7516

I need a constraint propagate framework to solve that problem for table partition.

What is changed and how it works?

The comment in the code describes an algorithm in detail.

Partition pruning will use the output of current constant propagate result, and continue to propagate more constraints to see whether a partition could be pruned.

Check List

Tests

  • Unit test

Side effects


This change is Reviewable

@shenli
Copy link
Member

shenli commented Sep 7, 2018

What's mean by in-equal conditions?

@tiancaiamao
Copy link
Contributor Author

Such as > < >= <= @shenli

Our current constant propagate can't infer the following, because it doesn't propagate conditions from > <:

a = b, b = c, c = d , a < 5, d > 6  => false

@winoros
Copy link
Member

winoros commented Sep 10, 2018

Why not put this after the current ConstantPropagation, so you don't need to consider propagate the in and eq conditions.
And we can make the optimization more flexible.
For example, we may extract new filters from ExtractFiltersFromDNFs, and the new extracted filter can be evaluated by this algo. If you combine the propagating and evaluating, it's hard to place ExtractFiltersFromDNFs to its right place.

And since this algo is about CNF why not break when finding some part is false.

@zz-jason
Copy link
Member

@tiancaiamao The code patch is too large, could you please provide a brief introduction about how this algorithm works? Or maybe a proposal?

@zz-jason zz-jason added sig/planner SIG: Planner type/enhancement The issue or PR belongs to an enhancement. labels Sep 10, 2018
@tiancaiamao
Copy link
Contributor Author

PTAL @zz-jason @eurekaka @winoros

1 similar comment
@tiancaiamao
Copy link
Contributor Author

PTAL @zz-jason @eurekaka @winoros

Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

Why not put this after the current ConstantPropagation, so you don't need to consider propagate the in and eq conditions.
And we can make the optimization more flexible.
For example, we may extract new filters from ExtractFiltersFromDNFs, and the new extracted filter can be evaluated by this algo. If you combine the propagating and evaluating, it's hard to place ExtractFiltersFromDNFs to its right place.

And since this algo is about CNF why not break when finding some part is false.

+1

Besides, we propagate filters over outer join now, for this situation, we need to differentiate join conditions and filter conditions, conditions from left child schema and right child schema, it looks hard to fit this into fixPoint. I prefer using this framework for expression simplification as mentioned in PR description, e.g, a > 1 and a > 2 => a > 2.

expression/new_constant_propagation.go Outdated Show resolved Hide resolved
expression/new_constant_propagation.go Outdated Show resolved Hide resolved
expression/new_constant_propagation.go Outdated Show resolved Hide resolved
expression/new_constant_propagation.go Outdated Show resolved Hide resolved
@tiancaiamao
Copy link
Contributor Author

PTAL @eurekaka @winoros @zz-jason

@tiancaiamao
Copy link
Contributor Author

PTAL @eurekaka @winoros

@eurekaka
Copy link
Contributor

My concern stated above has not been addressed yet, i.e, where to use this framework? if we are going to use it to substitute current constant propagation, then the problems we mentioned above should be addressed? if this is just for partition pruning, then LGTM now and I am going to shut up.

@tiancaiamao
Copy link
Contributor Author

Sorry for the late reply, I'm far too busy recently.

where to use this framework?

In table partition pruning, of course, that's also the primary reason it's written.

If you'll address the problem for non-equal condition and monotonic function using current constant propagate algorithm (or give me a promise to do it in the foreseeable future), I'd like to close this PR and wait for your solution. Otherwise, I have to do it myself.

If the proposed algorithm is general enough, I'd like to try to substitute current constant propagation to make the code base simpler.

If it's not or, due to time limitation, it could still works with current algorithm, they are not conflict (correct me if I am wrong).

@eurekaka

@tiancaiamao
Copy link
Contributor Author

PTAL @eurekaka @winoros @zz-jason

@tiancaiamao
Copy link
Contributor Author

PTAL @eurekaka @winoros @zz-jason

@eurekaka
Copy link
Contributor

if this is just for partition pruning, then LGTM now and I am going to shut up.

In table partition pruning, of course, that's also the primary reason it's written.

So LGTM.

If the proposed algorithm is general enough, I'd like to try to substitute current constant propagation to make the code base simpler.

That would be great.

@eurekaka eurekaka added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 16, 2018
@tiancaiamao
Copy link
Contributor Author

PTAL @winoros @zz-jason

2 similar comments
@tiancaiamao
Copy link
Contributor Author

PTAL @winoros @zz-jason

@tiancaiamao
Copy link
Contributor Author

PTAL @winoros @zz-jason

@tiancaiamao
Copy link
Contributor Author

PTAL @winoros @zz-jason

@zz-jason zz-jason added the require-LGT3 Indicates that the PR requires three LGTM. label Nov 22, 2018
expression/new_constant_propagation.go Outdated Show resolved Hide resolved
expression/new_constant_propagation.go Outdated Show resolved Hide resolved
@zz-jason
Copy link
Member

@tiancaiamao If we treat to_days(t) as a special column, then we can handle these kind of constant propagation in the old framework.

@zz-jason
Copy link
Member

And I prefer to change the name from constant propagation to constraint propagation

@tiancaiamao
Copy link
Contributor Author

If we treat to_days(t) as a special column, then we can handle these kind of constant propagation in the old framework.

Sounds not bad.

If you'll address the problem for non-equal condition and monotonic function using current constant propagate algorithm, I'd like to close this PR and wait for your solution. Otherwise, I have to do it myself.

Then I'll push you to do that. @zz-jason

@tiancaiamao tiancaiamao removed the require-LGT3 Indicates that the PR requires three LGTM. label Nov 22, 2018
@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Nov 22, 2018

I remove the require-LGTM3 tag, which I think is not reasonable @zz-jason
If this PR involves significant changes, or maybe cause data loss, DDL failure, transaction bugs ...
Feel free to add the tag back, and then I'll require more reviewer for this issue.

@tiancaiamao
Copy link
Contributor Author

Friendly ping @zz-jason

@tiancaiamao tiancaiamao changed the title expression: a new framework for more aggressive constant propagate expression: a constraint propagate framework mainly for partition pruning Nov 28, 2018
@tiancaiamao
Copy link
Contributor Author

I've update the description, PTAL @zz-jason @eurekaka

Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 28, 2018
@zz-jason
Copy link
Member

/run-all-tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression sig/planner SIG: Planner status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants