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

expression: forbidden bind sql when subquery or union exists #27347

Merged
merged 30 commits into from
Aug 21, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
061ac16
find bug pos, we should get table list from subquery recursive
sylzd Aug 17, 2021
81b9b49
add visitor
sylzd Aug 18, 2021
1c36a1a
done
sylzd Aug 18, 2021
b122f1f
fix
sylzd Aug 18, 2021
c825c54
Merge branch 'master' into fix_26377_2
sylzd Aug 18, 2021
7ac1781
Merge branch 'master' into fix_26377_2
sylzd Aug 18, 2021
56cce2e
fix gosimple
sylzd Aug 18, 2021
5966bc2
Merge branch 'fix_26377_2' of github.com:sylzd/tidb into fix_26377_2
sylzd Aug 18, 2021
02593fb
fix
sylzd Aug 18, 2021
78bb3b1
fix
sylzd Aug 18, 2021
5b4a7ae
fix
sylzd Aug 18, 2021
151e2cd
simplify
sylzd Aug 18, 2021
1e0142c
happy lint
sylzd Aug 18, 2021
c21f251
add local tests
sylzd Aug 19, 2021
81ce253
add some test cases
sylzd Aug 19, 2021
025253d
add test cases
sylzd Aug 19, 2021
ba379e7
make check
sylzd Aug 19, 2021
9d2d3b5
add cases about column subquery
sylzd Aug 19, 2021
7306096
make lint happy
sylzd Aug 19, 2021
5955ade
Merge branch 'master' into fix_26377_2
ti-chi-bot Aug 20, 2021
4c6de4d
Merge branch 'master' into fix_26377_2
sylzd Aug 20, 2021
f46cedd
Merge branch 'master' into fix_26377_2
sylzd Aug 20, 2021
fa28bb8
Merge branch 'master' into fix_26377_2
sylzd Aug 20, 2021
be844d5
Merge branch 'master' into fix_26377_2
ti-chi-bot Aug 20, 2021
0500429
Merge branch 'master' into fix_26377_2
ti-chi-bot Aug 20, 2021
9554423
Merge branch 'master' into fix_26377_2
sylzd Aug 20, 2021
3132c1a
Merge branch 'master' into fix_26377_2
ti-chi-bot Aug 20, 2021
89cf8d2
Merge branch 'master' into fix_26377_2
ti-chi-bot Aug 20, 2021
b0af726
Merge branch 'master' into fix_26377_2
ti-chi-bot Aug 20, 2021
bdb423c
Merge branch 'master' into fix_26377_2
ti-chi-bot Aug 20, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix gosimple
  • Loading branch information
sylzd committed Aug 18, 2021
commit 56cce2e6bf427b03999a79c4ab09228043186def
2 changes: 1 addition & 1 deletion planner/core/logical_plan_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -5882,7 +5882,7 @@ func buildWindowSpecs(specs []ast.WindowSpec) (map[string]*ast.WindowSpec, error
func extractTableList(node ast.ResultSetNode, input []*ast.TableName, asName bool) []*ast.TableName {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can add a unit test for extractTableList ?

Copy link
Contributor Author

@sylzd sylzd Aug 19, 2021

Choose a reason for hiding this comment

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

That's a big project. It seems most internal methods in logic_plan_builder.go do not have a UT, such as extractTableSourceAsNames , extractOnCondition .... and so on. I think it's probably tested by high level.

Copy link
Collaborator

@lcwangchao lcwangchao Aug 20, 2021

Choose a reason for hiding this comment

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

Adding a unit test is a better practice to prevent explosion of combinations. So even we have some integration tests we still need unit tests to ensure robustness of our code. But I will approve the pr because it is for forbidden bind sql when subquery or union exists . We may add a unit test for this method in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree to make a specific project to improve the test coverage about logic_plan_builder.go

switch x := node.(type) {
case *ast.SubqueryExpr:
input = extractTableList(x.Query.(ast.ResultSetNode), input, asName)
input = extractTableList(x.Query, input, asName)
case *ast.SelectStmt:
input = extractTableList(x.From.TableRefs.Left, input, asName)
input = extractTableList(x.From.TableRefs.Right, input, asName)
Expand Down
2 changes: 1 addition & 1 deletion planner/core/preprocess.go
Original file line number Diff line number Diff line change
Expand Up @@ -417,11 +417,11 @@ func (p *preprocessor) checkBindGrammar(originNode, hintedNode ast.StmtNode, def
resNode = n
case *ast.SetOprStmt:
resNode = n
//TODO: What about insert into (select * from t)
case *ast.DeleteStmt:
resNode = n.TableRefs.TableRefs
case *ast.UpdateStmt:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should also check where expr in Delete/Update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's open a new issue about Delete/Update/Insert~

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok~ I have created a new issue #27422

Copy link
Contributor Author

@sylzd sylzd Aug 20, 2021

Choose a reason for hiding this comment

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

thx a lot, I'm working on it.

resNode = n.TableRefs.TableRefs
//TODO: What about insert into (select * from t)
case *ast.InsertStmt:
resNode = n.Table.TableRefs
}
Expand Down