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

plan: implement ExplainInfo() interface for simple operators #3883

Merged
merged 7 commits into from Jul 26, 2017

Conversation

zz-jason
Copy link
Member

to #3805

@zz-jason zz-jason removed DNM labels Jul 26, 2017
@zz-jason
Copy link
Member Author

@hanfei1991 @lamxTyler @winoros PTAL

@@ -530,6 +530,18 @@ const (
CompleteAgg
)

func (at AggregationType) String() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment for the exported function.
Also, it seems that this function is not used?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is used by PhysicalAggregation.ExplainInfo()

@@ -52,7 +52,7 @@ func (s *testSuite) TestAnalyzeTable(c *C) {
tk.MustExec("analyze table t1 index ind_a")
result = tk.MustQuery("explain select * from t1 where t1.a = 1")
rowStr = fmt.Sprintf("%s", result.Rows())
c.Check(strings.Split(rowStr, "{")[0], Equals, "[[TableScan_4 Selection_5 cop ] [Selection_5 cop ] [TableReader_6 root ]]")
c.Check(strings.Split(rowStr, "{")[0], Equals, "[[TableScan_4 Selection_5 cop ] [Selection_5 cop eq(col(test.t1.a), const(1))] [TableReader_6 root ]]")
Copy link
Contributor

Choose a reason for hiding this comment

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

The Split is unnecessary now, the same with line 42, 46.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -52,7 +52,7 @@ func (s *testSuite) TestAnalyzeTable(c *C) {
tk.MustExec("analyze table t1 index ind_a")
result = tk.MustQuery("explain select * from t1 where t1.a = 1")
rowStr = fmt.Sprintf("%s", result.Rows())
c.Check(strings.Split(rowStr, "{")[0], Equals, "[[TableScan_4 Selection_5 cop ] [Selection_5 cop ] [TableReader_6 root ]]")
c.Check(rowStr, Equals, "[[TableScan_4 Selection_5 cop ] [Selection_5 cop eq(col(test.t1.a), const(1))] [TableReader_6 root ]]")
Copy link
Member

Choose a reason for hiding this comment

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

I think col(blabla..), constant(blabla...) is redundant...

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
Copy link
Member Author

@hanfei1991 @lamxTyler PTAL again

Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@alivxxx alivxxx added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 26, 2017
@zz-jason zz-jason added Priority/P1 Features that will be implemented in the latest or next major/minor version non-release-blocker labels Jul 26, 2017
Copy link
Member

@hanfei1991 hanfei1991 left a comment

Choose a reason for hiding this comment

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

LGTM

@hanfei1991 hanfei1991 merged commit c0c9519 into master Jul 26, 2017
@hanfei1991 hanfei1991 deleted the 3805-explain-simple branch July 26, 2017 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority/P1 Features that will be implemented in the latest or next major/minor version status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants