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

*: Support stream aggregation in new plan #4481

Merged
merged 15 commits into from Sep 14, 2017

Conversation

Projects
None yet
5 participants
@zimulala
Member

zimulala commented Sep 8, 2017

support stream aggregation in the new plan and clean the tests.

@@ -380,7 +380,7 @@ func (s *testPlanSuite) TestDAGPlanBuilderSubquery(c *C) {
// Test Nested sub query.
{

This comment has been minimized.

@hanfei1991

hanfei1991 Sep 8, 2017

Member

add test for stream agg + index join and stream agg + merge join and stream agg + limit and stream agg + sort

@hanfei1991

hanfei1991 Sep 8, 2017

Member

add test for stream agg + index join and stream agg + merge join and stream agg + limit and stream agg + sort

This comment has been minimized.

@zimulala

zimulala Sep 12, 2017

Member

Done.

@zimulala
Show outdated Hide outdated plan/logical_plans.go
Show outdated Hide outdated plan/logical_plans.go
Show outdated Hide outdated plan/new_physical_plan_builder.go
return nil
}
}
// group by a + b is not interested in any order.

This comment has been minimized.

@hanfei1991

hanfei1991 Sep 8, 2017

Member

Add test for this case.

@hanfei1991

hanfei1991 Sep 8, 2017

Member

Add test for this case.

This comment has been minimized.

@zimulala

zimulala Sep 12, 2017

Member

Done.

@zimulala
Show outdated Hide outdated plan/task.go
Show outdated Hide outdated plan/stats.go

@pingcap pingcap deleted a comment from hanfei1991 Sep 8, 2017

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli
Member

shenli commented Sep 9, 2017

},
{
sql: "select count(*) from t where e > 1 group by b",
best: "TableReader(Table(t)->Sel([gt(test.t.e, 1)])->HashAgg)->HashAgg",

This comment has been minimized.

@shenli

shenli Sep 9, 2017

Member

Why not use index on e for filtering or use index on b for streaming aggregation?

@shenli

shenli Sep 9, 2017

Member

Why not use index on e for filtering or use index on b for streaming aggregation?

This comment has been minimized.

@hanfei1991

hanfei1991 Sep 10, 2017

Member

using index will cause a double read.

@hanfei1991

hanfei1991 Sep 10, 2017

Member

using index will cause a double read.

zimulala added some commits Sep 12, 2017

@hanfei1991

This comment has been minimized.

Show comment
Hide comment
@hanfei1991

hanfei1991 Sep 12, 2017

Member

LGTM

Member

hanfei1991 commented Sep 12, 2017

LGTM

@winoros

rest LGTM

best: "IndexLookUp(Index(t.b_c)[[-inf <nil>,20 +inf]], Table(t)->HashAgg)->HashAgg",
},
{
sql: "select count(e) from t where t.b <= 30",

This comment has been minimized.

@winoros

winoros Sep 13, 2017

Member

the case that t.b <= 30 and t.b <= 40 is not necessary?

@winoros

winoros Sep 13, 2017

Member

the case that t.b <= 30 and t.b <= 40 is not necessary?

This comment has been minimized.

@zimulala

zimulala Sep 13, 2017

Member

There are old tests. @hanfei1991

@zimulala

zimulala Sep 13, 2017

Member

There are old tests. @hanfei1991

This comment has been minimized.

@hanfei1991
@hanfei1991
@winoros

This comment has been minimized.

Show comment
Hide comment
@winoros

winoros Sep 13, 2017

Member

/run-all-test

Member

winoros commented Sep 13, 2017

/run-all-test

@zimulala

This comment has been minimized.

Show comment
Hide comment
@zimulala
Member

zimulala commented Sep 13, 2017

PTAL @winoros

@winoros

LGTM

@winoros winoros added status/LGT2 and removed status/LGT1 labels Sep 14, 2017

@XuHuaiyu XuHuaiyu merged commit d0be70d into master Sep 14, 2017

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@XuHuaiyu XuHuaiyu deleted the zimuxia/stream-agg branch Sep 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment