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.

Sign up for GitHub

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

Jump to bottom

expression: implement vectorized evaluation for builtinJSONContainsPathSig #13956

Merged
merged 9 commits into from
Jan 7, 2020
Merged

expression: implement vectorized evaluation for builtinJSONContainsPathSig #13956

merged 9 commits into from
Jan 7, 2020

Conversation

chenlx0
Copy link
Contributor

@chenlx0 chenlx0 commented Dec 7, 2019 •
edited
Loading

PCP #12103

What problem does this PR solve?

implement vectorized evaluation for builtinJSONContainsPathSig

What is changed and how it works?

About 5% faster.

make vectorized-bench VB_FILE=JSON VB_FUNC=builtinJSONContainsPathSig
cd ./expression && \
                go test -v -benchmem \
                        -bench=BenchmarkVectorizedBuiltinJSONFunc \
                        -run=BenchmarkVectorizedBuiltinJSONFunc \
                        -args "builtinJSONContainsPathSig"
goos: darwin
goarch: amd64
pkg: github.com/pingcap/tidb/expression
BenchmarkVectorizedBuiltinJSONFunc/builtinJSONContainsPathSig-VecBuiltinFunc-8              1492            778357 ns/op          382340 B/op       3074 allocs/op
BenchmarkVectorizedBuiltinJSONFunc/builtinJSONContainsPathSig-NonVecBuiltinFunc-8           1448            829472 ns/op          382058 B/op       3073 allocs/op
BenchmarkVectorizedBuiltinJSONFunc/builtinJSONContainsPathSig-VecBuiltinFunc#01-8           1462            807511 ns/op          382373 B/op       3074 allocs/op
BenchmarkVectorizedBuiltinJSONFunc/builtinJSONContainsPathSig-NonVecBuiltinFunc#01-8        1371            875612 ns/op          382369 B/op       3073 allocs/op
BenchmarkVectorizedBuiltinJSONFunc/builtinJSONContainsPathSig-VecBuiltinFunc#02-8           1226            950570 ns/op          533717 B/op       5122 allocs/op
BenchmarkVectorizedBuiltinJSONFunc/builtinJSONContainsPathSig-NonVecBuiltinFunc#02-8        1209           1007417 ns/op          533836 B/op       5121 allocs/op
BenchmarkVectorizedBuiltinJSONFunc/builtinJSONContainsPathSig-VecBuiltinFunc#03-8           1410            857041 ns/op          386513 B/op       4098 allocs/op
BenchmarkVectorizedBuiltinJSONFunc/builtinJSONContainsPathSig-NonVecBuiltinFunc#03-8        1300            912372 ns/op          386687 B/op       4097 allocs/op
PASS
ok      github.com/pingcap/tidb/expression      10.409s

Check List

Tests

  • Unit test

@chenlx0 chenlx0 requested a review from a team as a code owner December 7, 2019 07:51
@sre-bot
Copy link
Contributor

sre-bot commented Dec 7, 2019

Thanks for your contribution. If your PR get merged, you will be rewarded 50 points.

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 7, 2019
@ghost ghost requested review from qw4990 and XuHuaiyu and removed request for a team December 7, 2019 07:51
@XuHuaiyu
Copy link
Contributor

Test failed.

[2019-12-16T11:09:16.926Z] FAIL: integration_test.go:3900: testIntegrationSuite.TestFuncJSON
[2019-12-16T11:09:16.926Z] 
[2019-12-16T11:09:16.926Z] integration_test.go:4022:
[2019-12-16T11:09:16.926Z]     r = tk.MustQuery(`select
[2019-12-16T11:09:16.926Z]     		json_contains_path(NULL, 'one', "$.c"),
[2019-12-16T11:09:16.926Z]     		json_contains_path(NULL, 'all', "$.c"),
[2019-12-16T11:09:16.926Z]     		json_contains_path('{"a": 1}', NULL, "$.c"),
[2019-12-16T11:09:16.926Z]     		json_contains_path('{"a": 1}', 'one', NULL),
[2019-12-16T11:09:16.926Z]     		json_contains_path('{"a": 1}', 'all', NULL)
[2019-12-16T11:09:16.926Z]     	`)

@chenlx0 chenlx0 requested a review from a team as a code owner December 18, 2019 05:40
@ghost ghost requested review from francis0407 and alivxxx and removed request for a team December 18, 2019 05:40
@chenlx0 chenlx0 force-pushed the master branch 2 times, most recently from a5149e1 to 0fd3f29 Compare December 18, 2019 05:46
@francis0407 francis0407 removed their request for review December 18, 2019 05:50
@alivxxx alivxxx removed their request for review December 18, 2019 07:08
@chenlx0
Copy link
Contributor Author

chenlx0 commented Dec 18, 2019

Test failed.

[2019-12-16T11:09:16.926Z] FAIL: integration_test.go:3900: testIntegrationSuite.TestFuncJSON
[2019-12-16T11:09:16.926Z] 
[2019-12-16T11:09:16.926Z] integration_test.go:4022:
[2019-12-16T11:09:16.926Z]     r = tk.MustQuery(`select
[2019-12-16T11:09:16.926Z]     		json_contains_path(NULL, 'one', "$.c"),
[2019-12-16T11:09:16.926Z]     		json_contains_path(NULL, 'all', "$.c"),
[2019-12-16T11:09:16.926Z]     		json_contains_path('{"a": 1}', NULL, "$.c"),
[2019-12-16T11:09:16.926Z]     		json_contains_path('{"a": 1}', 'one', NULL),
[2019-12-16T11:09:16.926Z]     		json_contains_path('{"a": 1}', 'all', NULL)
[2019-12-16T11:09:16.926Z]     	`)

All problems result in test failed fixed, PTAL @XuHuaiyu

qw4990
qw4990 reviewed Dec 23, 2019
View reviewed changes
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

Sorry for my late reply, LGTM

@qw4990
Copy link
Contributor

qw4990 commented Dec 23, 2019

/run-all-tests

@qw4990 qw4990 added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 23, 2019
wshwsh12
wshwsh12 reviewed Dec 25, 2019
View reviewed changes
expression/builtin_json_vec.go Show resolved Hide resolved
expression/builtin_json_vec.go Outdated Show resolved Hide resolved
expression/builtin_json_vec.go Show resolved Hide resolved
@qw4990
Copy link
Contributor

qw4990 commented Dec 25, 2019

@chenlx0 Friendly ping, please address comments.

@sre-bot
Copy link
Contributor

sre-bot commented Jan 4, 2020

@chenlx0, please update your pull request.

@qw4990 qw4990 requested a review from wshwsh12 January 7, 2020 08:36
wshwsh12
wshwsh12 approved these changes Jan 7, 2020
View reviewed changes
Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

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

LGTM

@wshwsh12 wshwsh12 added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. status/ReqChange labels Jan 7, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jan 7, 2020

/run-all-tests

@sre-bot sre-bot merged commit a486113 into pingcap:master Jan 7, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jan 7, 2020

Team blueshit complete task #12103 and get 50 score, currerent score 350.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XuHuaiyu XuHuaiyu XuHuaiyu left review comments

@qw4990 qw4990 qw4990 left review comments

@wshwsh12 wshwsh12 wshwsh12 approved these changes

Assignees
No one assigned
Labels
component/expression contribution This PR is from a community contributor. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@chenlx0 @sre-bot @XuHuaiyu @qw4990 @wshwsh12 @zz-jason