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

planner/cascades: Add the transformation rule PushSelDownWindow #14068

Merged
merged 4 commits into from
Dec 16, 2019

Conversation

jiangyuzhao
Copy link
Contributor

@jiangyuzhao jiangyuzhao commented Dec 16, 2019

What problem does this PR solve?

This PR adds Transformation rule PushSelDownWindow which is a part of predicate push down in cascades planner(#13709).

What is changed and how it works?

The logic is the same with planner/core/rule_predicate_push_down.go/LogicalWindow.PredicatePushdown.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

… push OperandSelection down to OperandWindow.
@jiangyuzhao jiangyuzhao requested a review from a team as a code owner December 16, 2019 05:26
@ghost ghost requested review from francis0407 and lzmhhh123 and removed request for a team December 16, 2019 05:26
@claassistantio
Copy link

claassistantio commented Dec 16, 2019

CLA assistant check
All committers have signed the CLA.

@winoros winoros added sig/planner SIG: Planner contribution This PR is from a community contributor. labels Dec 16, 2019
@winoros
Copy link
Member

winoros commented Dec 16, 2019

@jiangyuzhao Oh, there's one more thing you need to modify:

func (s *testTransformationRuleSuite) SetUpSuite(c *C) {
	s.is = infoschema.MockInfoSchema([]*model.TableInfo{plannercore.MockSignedTable()})
	s.sctx = plannercore.MockContext()
	s.Parser = parser.New()
	s.optimizer = NewOptimizer()
	var err error
	s.testData, err = testutil.LoadTestSuiteData("testdata", "transformation_rules_suite")
	c.Assert(err, IsNil)
}

After s.Parser = parser.New(). We need to set p.EnableWindowFunc(true) to enable window function in test.

@lzmhhh123 lzmhhh123 changed the title planner/cascades: fix #13709. Add a transformation rule about push O… planner/cascades: Add the transformation rule PushSelDownWindow Dec 16, 2019
@jiangyuzhao
Copy link
Contributor Author

@jiangyuzhao Oh, there's one more thing you need to modify:

func (s *testTransformationRuleSuite) SetUpSuite(c *C) {
	s.is = infoschema.MockInfoSchema([]*model.TableInfo{plannercore.MockSignedTable()})
	s.sctx = plannercore.MockContext()
	s.Parser = parser.New()
	s.optimizer = NewOptimizer()
	var err error
	s.testData, err = testutil.LoadTestSuiteData("testdata", "transformation_rules_suite")
	c.Assert(err, IsNil)
}

After s.Parser = parser.New(). We need to set p.EnableWindowFunc(true) to enable window function in test.

I think it will be better if I use

s.Parser.EnableWindowFunc(true)
defer func() {
	s.Parser.EnableWindowFunc(false)
}()

in TestPredicatePushDown?

@winoros
Copy link
Member

winoros commented Dec 16, 2019

@jiangyuzhao Oh, there's one more thing you need to modify:

```

func (s *testTransformationRuleSuite) SetUpSuite(c *C) {
s.is = infoschema.MockInfoSchema([]*model.TableInfo{plannercore.MockSignedTable()})
s.sctx = plannercore.MockContext()
s.Parser = parser.New()
s.optimizer = NewOptimizer()
var err error
s.testData, err = testutil.LoadTestSuiteData("testdata", "transformation_rules_suite")
c.Assert(err, IsNil)
}



After `s.Parser = parser.New()`. We need to set `p.EnableWindowFunc(true)` to enable window function in test.

I think it will be better if I use

s.Parser.EnableWindowFunc(true)
defer func() {
	s.Parser.EnableWindowFunc(false)
}()

in TestPredicatePushDown?

It's ok to open it in SetUpSuite.

Copy link
Member

@francis0407 francis0407 left a comment

Choose a reason for hiding this comment

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

Thanks for you contribution.
LGTM after address this comment.

planner/cascades/transformation_rules.go Outdated Show resolved Hide resolved
"Group#3 Schema:[test.t.a,test.t.b,Column#14]",
" Window_4 input:[Group#4]",
"Group#4 Schema:[test.t.a,test.t.b]",
" Projection_3 input:[Group#5], test.t.a, test.t.b",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure whether it's right. For the Projection_3 may be unexpected.
And I wonder why we can only push b > 10 down to window, that is why we can only push col in partition by down to window?

Copy link
Member

Choose a reason for hiding this comment

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

Let me explain the second one first.
Push the filters which contain the column not in partition by clause would cause wrong result.
Take the given case as example. There're two rows a=-1 and b = 11 and a = 10 and b = 11. If we push the filter a < 10 down to the window. a=-1 and b=11 will be filtered. Then the partition b=11's min(a) would become 10 instead of -1.

Copy link
Member

@winoros winoros Dec 16, 2019

Choose a reason for hiding this comment

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

And for the first one.
Though windown is in the select field but it's calculated at last.
For a SQL pattern select normal_field, window_field from tables where filters:
The tables in FROM CLAUSE is calcualted first, then is the normal_field, then filters, then window_field. So some redundant project may be added when build plans. We'll use the EliminateProjection rule to eliminate the unnecessary ones.
@jingyugao

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, Thank you for the awesome explanation. But my github name is @jiangyuzhao , hhh.

@codecov
Copy link

codecov bot commented Dec 16, 2019

Codecov Report

Merging #14068 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master    #14068   +/-   ##
=========================================
  Coverage   80.449%   80.449%           
=========================================
  Files          483       483           
  Lines       123452    123452           
=========================================
  Hits         99316     99316           
  Misses       16355     16355           
  Partials      7781      7781

@winoros
Copy link
Member

winoros commented Dec 16, 2019

Thanks, please sign the CLA. See @claassistantio 's comment

Copy link
Member

@francis0407 francis0407 left a comment

Choose a reason for hiding this comment

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

LGTM

@francis0407 francis0407 added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 16, 2019
@jiangyuzhao
Copy link
Contributor Author

@lzmhhh123

Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM

@lzmhhh123 lzmhhh123 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 16, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 16, 2019

/run-all-tests

@sre-bot sre-bot merged commit 80b9d87 into pingcap:master Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants