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: support building data source from View #8757

Merged
merged 12 commits into from
Dec 26, 2018

Conversation

AndrewDi
Copy link
Contributor

@AndrewDi AndrewDi commented Dec 19, 2018

What problem does this PR solve?

Implement basic select view feature

What is changed and how it works?

ref proposal Proposal: Implement View Feature

Check List

Tests

  • Unit test

This change is Reviewable

@AndrewDi
Copy link
Contributor Author

/rebuild

planner/core/logical_plan_builder.go Outdated Show resolved Hide resolved
planner/core/logical_plan_builder.go Outdated Show resolved Hide resolved
planner/core/logical_plan_builder.go Outdated Show resolved Hide resolved
@XuHuaiyu
Copy link
Contributor

Add a test case for changing column order of the underlying table.

create table t(a int, b int);
create view v as select * from t;
alter table t drop column a;
alter table t add column a int after b;
select * from v;

@XuHuaiyu XuHuaiyu added contribution This PR is from a community contributor. sig/planner SIG: Planner type/new-feature labels Dec 20, 2018
@AndrewDi
Copy link
Contributor Author

@XuHuaiyu PTAL

if col == nil {
return nil, ErrViewInvalid.GenWithStackByArgs(dbName.L, tableInfo.Name.L)
}
col.ColName = tableInfo.Cols()[i].Name
Copy link
Contributor

Choose a reason for hiding this comment

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

line2018 and line2019 is useless.

planner/core/logical_plan_builder.go Outdated Show resolved Hide resolved
planner/core/logical_plan_builder.go Outdated Show resolved Hide resolved
}
projUponView := LogicalProjection{Exprs: expression.Column2Exprs(viewCols)}.Init(b.ctx)
projUponView.SetChildren(selectLogicalPlan.(LogicalPlan))
projUponView.SetSchema(expression.NewSchema(viewCols...))
Copy link
Contributor

Choose a reason for hiding this comment

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

We should build schema.Columns instead of using the selectLogicalPlan.Schema.Columns

&expression.Column{
		UniqueID:    b.ctx.GetSessionVars().AllocPlanColumnID(),
		TblName:     viewname,
		OrigTblName: tableInfo.Cols[i].Name,
		ColName:     colName,
		OrigColName: tableInfo.View.Cols[i].Name,
		DBName:      dbName,
		RetType:     expr.GetType(),
	}

Copy link
Contributor

Choose a reason for hiding this comment

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

We should not use the column in the child schema as the parent schema

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understand, PTAL again

XuHuaiyu
XuHuaiyu previously approved these changes Dec 20, 2018
executor/executor_test.go Outdated Show resolved Hide resolved
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

Please also add tests in planner package.

planner/core/logical_plan_builder.go Outdated Show resolved Hide resolved
Copy link
Contributor

@XuHuaiyu XuHuaiyu 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

@winoros winoros left a comment

Choose a reason for hiding this comment

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

LGTM

winoros
winoros previously approved these changes Dec 24, 2018
@XuHuaiyu
Copy link
Contributor

/run-all-tests

@XuHuaiyu XuHuaiyu added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 25, 2018
@XuHuaiyu
Copy link
Contributor

/rebuild

@@ -1994,6 +1998,46 @@ func (b *PlanBuilder) buildDataSource(tn *ast.TableName) (LogicalPlan, error) {
return result, nil
}

func (b *PlanBuilder) buildDataSourceFromView(dbName model.CIStr, tableInfo *model.TableInfo) (LogicalPlan, error) {
var (
Copy link
Member

Choose a reason for hiding this comment

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

These variable declaration looks ugly. We can avoid this by using the := operator in golang.

planner/core/logical_plan_builder.go Outdated Show resolved Hide resolved
planner/core/logical_plan_builder.go Outdated Show resolved Hide resolved
@zz-jason
Copy link
Member

/run-all-tests

@AndrewDi
Copy link
Contributor Author

@zz-jason PTAL

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 changed the title planner,executor: basic support for SELECT_VIEW planner: support building data source from View Dec 26, 2018
@XuHuaiyu XuHuaiyu merged commit 123aba2 into pingcap:master Dec 26, 2018
@AndrewDi AndrewDi deleted the view/implement_select_view branch December 26, 2018 12:24
yu34po pushed a commit to yu34po/tidb that referenced this pull request Jan 2, 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/LGT2 Indicates that a PR has LGTM 2. type/new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants