Keep ast's wildcard expanded after building LogicalPlan

[AndrewDi]

Feature Request

Is your feature request related to a problem? Please describe: In implement view feature, SQL from view's definition need to be expanded if sql contains any wildcard,

Describe the feature you'd like: Because table column info mostly can be obtained during build logical plan，so I think how about we keep ast's wildcard expanded after logical plan building complete? And current behavior is to reset ast's fields to original ast's fileds after building logical plan, following is the code reference: https://github.com/pingcap/tidb/blob/6a9ad3fe25e8c835da2dfc83afd458748c92c9ac/planner/core/logical_plan_builder.go#L1806 https://github.com/pingcap/tidb/blob/6a9ad3fe25e8c835da2dfc83afd458748c92c9ac/planner/core/logical_plan_builder.go#L1907

Describe alternatives you've considered:

Teachability, Documentation, Adoption, Migration Strategy:

[AndrewDi]

@XuHuaiyu PTAL

[XuHuaiyu]

The code is introduced in https://github.com/pingcap/tidb/pull/2767. Please check whether there will be negative effects if we do the modification.

[AndrewDi]

@XuHuaiyu Do we have any test case to reproduce this problem, Following test case from PR #2767 runs fine.

create table multiexec (a int, b int);
insert multiexec values (1, 1), (2, 2);
prepare stmt from 'select a from multiexec where b = ? order by b';
set @val=1;
execute stmt using @val;
set @val=2;
execute stmt using @val;
deallocate prepare stmt;
drop table multiexec;

[XuHuaiyu]

Have you checked the result of make dev?

[AndrewDi]

I run make dev with https://github.com/AndrewDi/tidb/tree/view/keep_ast_expand branch on CircleCI and success.

[XuHuaiyu]

Yep, the panic mentioned in that issue may not happen, since we've removed the corresponded code line.

I think it's ok to make this modification.

PTAL @winoros @zz-jason

[winoros]

It changed the AST tree itself. When will the ast.Node.Restore be called? I'm not sure whether this will break its functionality.

[AndrewDi]

@winoros I may call ast.Node.Restore when Create View.

[XuHuaiyu]

@winoros It seems that we do not re-use the AST now.

[XuHuaiyu]

@AndrewDi I wonder whether we need this to help us to perform the P1 tasks mentioned in https://github.com/pingcap/tidb/blob/master/docs/design/2018-10-24-view-support.md? This proposal only considers the SIMPLEST functions of view. We need another detailed proposal and discussion on other functions.

[winoros]

I don't know whether it's good behavior that modifying AST when building plan. In my own opinion, we can do this and remember to add some comments in a conspicuous place.

[AndrewDi]

@XuHuaiyu P1 task is almost finish, If I could get Expanded AST Tree, After ast2sql func complete, I think we just make SELECT FROM VIEW more compatibility with mysql.

[AndrewDi]

IMO if I want to get a Expanded AST Tree, there are two ways:

1. Modify the origin one ---If we do not reuse origin ast tree, this way is more simple. and it seems like mysql does the same way.
2. Build a new one ---I think it's not necessary。

[XuHuaiyu]

@winoros TiDB changes part of the AST now, like here.

[XuHuaiyu]

@AndrewDi You're right, but detailed investigation needs to be done before coding. Since no one really ensures which way is better. And advanced functions of view are complicated.

[AndrewDi]

@XuHuaiyu Yes, I just need modify SelectStmt.Fields.Fields, so I think it's not a hard decision.

[AndrewDi]

@XuHuaiyu @winoros Restore SelectStmt to SQL func completed, how about discuss this issue again? view/keep_ast_expand