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

executor: support Chunk for TableDualExec #5395

Merged
merged 22 commits into from
Dec 22, 2017

Conversation

zz-jason
Copy link
Member

to #5261

@zz-jason zz-jason mentioned this pull request Dec 13, 2017
41 tasks
@coocood
Copy link
Member

coocood commented Dec 14, 2017

numDualRows is always 1.
If not done, w can just write one row, set done to true and return.

return nil
}
dualSchema := v.Schema()
if dualSchema.Len() == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Why change the schema?

Copy link
Member Author

Choose a reason for hiding this comment

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

the schema can not be empty, we need to use it to build a chunk for TableDualExec and append some value into it to indicate there exists some rows in that chunk.

Copy link
Member Author

Choose a reason for hiding this comment

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

this work should be done in plan building stage

@zz-jason zz-jason added the WIP label Dec 14, 2017
@zz-jason zz-jason removed the WIP label Dec 19, 2017
@zz-jason
Copy link
Member Author

/run-all-tests

@zz-jason
Copy link
Member Author

/run-all-tests

@zz-jason
Copy link
Member Author

/run-all-tests

@zz-jason
Copy link
Member Author

@hanfei1991 @coocood PTAL

@coocood
Copy link
Member

coocood commented Dec 21, 2017

@zz-jason
I think a better way to solve the problem is to define a noColumnLen in Chunk, then when we don't have any columns, we use this value for NumRows().

@shenli
Copy link
Member

shenli commented Dec 21, 2017

Please resolve the conflicts.

if e.Schema().Len() == 0 {
return a.handleNoDelayExecutor(goCtx, e, ctx, pi)
} else if proj, ok := e.(*ProjectionExec); ok && proj.calculateNoDelay {
Copy link
Member

Choose a reason for hiding this comment

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

Please comment this condition branch. Why we need this?

@@ -257,7 +270,11 @@ func (b *planBuilder) buildSet(v *ast.SetStmt) Plan {
vars.Value = ast.NewValueExpr(cn.Name.Name.O)
}
mockTablePlan := LogicalTableDual{}.init(b.ctx)
mockTablePlan.SetSchema(expression.NewSchema())
mockTablePlan.SetSchema(expression.NewSchema(&expression.Column{
Copy link
Member

Choose a reason for hiding this comment

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

Why this is changed.

@zz-jason
Copy link
Member Author

/run-all-tests

@zz-jason
Copy link
Member Author

/run-all-tests

@coocood
Copy link
Member

coocood commented Dec 22, 2017

LGTM

@coocood coocood added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 22, 2017
@@ -174,6 +186,7 @@ func (c *Chunk) Append(other *Chunk, begin, end int) {
dst.length++
}
}
c.numVirtualRows += other.numVirtualRows
Copy link
Contributor

Choose a reason for hiding this comment

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

why add all other.numVirtualRows? if we call Append more than once

Copy link
Member Author

Choose a reason for hiding this comment

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

a mistake, fixed

winkyao
winkyao previously approved these changes Dec 22, 2017
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@winkyao
Copy link
Contributor

winkyao commented Dec 22, 2017

/run-all-tests

@winkyao
Copy link
Contributor

winkyao commented Dec 22, 2017

/ok-to-test

@zimulala zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 22, 2017
@iamxy
Copy link
Member

iamxy commented Dec 22, 2017

/rebuild

@pingcap pingcap deleted a comment from winkyao Dec 22, 2017
@coocood
Copy link
Member

coocood commented Dec 22, 2017

/run-all-tests

p.SetChildren(dual)
p.self = p
p.SetSchema(expression.NewSchema())
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we remove this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

@zz-jason zz-jason merged commit 855df18 into pingcap:master Dec 22, 2017
@zz-jason zz-jason deleted the dev/chunk/tabledual branch December 22, 2017 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants