MultiRow Insert Optimization by ashudeep-sharma-zz · Pull Request #2422 · vitessio/vitess

ashudeep-sharma-zz · 2016-12-30T15:23:41Z

This Pull request contains the changes for Optimising the vindex based queries on look-up shard for Multi-Row Insert.
@sougou , Please take a look.

sougou · 2017-01-03T22:50:38Z

go/vt/vtgate/router.go

-func (rtr *Router) handlePrimary(vcursor *queryExecutor, vindexKey interface{}, colVindex *vindexes.ColumnVindex, bv map[string]interface{}, rowNum int) (ksid []byte, err error) {
-	if vindexKey == nil {
+func (rtr *Router) handlePrimary(vcursor *queryExecutor, vindexKeys []interface{}, colVindex *vindexes.ColumnVindex, bv map[string]interface{}, allShards []*topodatapb.ShardReference) (keyspaceIDs [][]byte, err error) {
+	if vindexKeys == nil {


I don't think this is possible. Were you able to get coverage to trigger this path?

sougou · 2017-01-03T22:53:04Z

go/vt/vtgate/router.go

+func (rtr *Router) handleNonPrimary(vcursor *queryExecutor, vindexKeys []interface{}, colVindex *vindexes.ColumnVindex, bv map[string]interface{}, ksids [][]byte) error {
 	if colVindex.Owned {
-		if vindexKey == nil {
+		if vindexKeys == nil {


This should also not be possible.

sougou · 2017-01-03T22:56:13Z

Nice work. Only couple of comments.

ashudeep-sharma-zz · 2017-01-04T09:06:29Z

Removed the redundant code in the latest commit.
Also after running code coverage analysis for vindexes, vtgate and planbuilder, i got the following coverage stats:

coverage: 82.8% of statements
ok github.com/youtube/vitess/go/vt/vtgate/vindexes 0.024s

coverage: 80.3% of statements
ok github.com/youtube/vitess/go/vt/vtgate 0.075s

coverage: 98.4% of statements
ok github.com/youtube/vitess/go/vt/vtgate/planbuilder 0.068s

sougou

I did a full code coverage analysis. Found test cases missing in a few places. Some of these tests have been missing from before. But it's better to add them now, so we don't regret later.

The general philosophy is that, if it can happen in production, then we need that code to be covered. There are a few errors that are impossible because there's higher level sanity checking. It's ok if those paths are not covered.

sougou · 2017-01-04T19:57:37Z

go/vt/vtgate/vindexes/binary.go

+			return false, fmt.Errorf("Binary.Verify: %v", err)
+		}
+		if bytes.Compare(data, ksids[rowNum]) != 0 {
+			return false, nil


Needs test case. Same with all other vindexes that have similar code.

sougou · 2017-01-04T20:01:40Z

go/vt/vtgate/vindexes/lookup_hash.go

-	return vind.lkp.Verify(vcursor, id, ksid)
+// Verify returns true if ids maps to ksids.
+func (vind *LookupHashUnique) Verify(vcursor VCursor, ids []interface{}, ksids [][]byte) (bool, error) {
+	return vind.lkp.Verify(vcursor, ids, ksids)


Needs test case. Same with the rest of the functions.

sougou · 2017-01-04T20:05:10Z

go/vt/vtgate/vindexes/lookup_internal.go

-	t := m["table"]
-	from := m["from"]
-	to := m["to"]
+func (lkp *lookup) Init(lookupQueryParams map[string]string, isHashed bool) {


Many functions in this file are missing critical code coverage. It's probably an artifact of previous changes.

sougou · 2017-01-04T20:15:32Z

go/vt/vtgate/engine/route.go

 				return err
 			}
-			if vindexKey == nil {
+			if vindexKeys == nil {


Is this possible?

sougou · 2017-01-04T20:15:54Z

go/vt/vtgate/engine/route.go

-			vindexKey, err = reversible.ReverseMap(vcursor, ksid)
+			vindexKeys, err = reversible.ReverseMap(vcursor, reverseKsids)
 			if err != nil {
 				return err


Missing test case for this. Is this possible?

ashudeep-sharma-zz · 2017-01-05T14:37:18Z

@sougou : Added corresponding positive as well as negative test-cases for all the vindexes. Please take a look

sougou · 2017-01-05T21:27:16Z

LGTM

Our internal implementation of Locate() does not support to be called from init(). Therefore, I've changed the test to create a new vindex in each test instance. I've also added error checking for the creation of the vindex to make sure we don't swallow any errors. I've fixed this code before in commit 1bcbff7 but I forgot to add a proper warning in the code to make sure that it doesn't happen again. Therefore, the same issue was introduced in vitessio#2422. With the new warning, people should no longer eagerly revert it back to a global variable :)

* 'master' of https://github.com/youtube/vitess: (220 commits) vtgate/vindexes: Do not run testfiles.Locate() from init() (2nd time). Add comment to document when new fields are included in response publish site Fri Jan 6 08:02:30 PST 2017 Updating doc. Optionally send additional C API fields through in responses Fixing explorer to handle empty files. MultiRow Insert Optimization (vitessio#2422) Now the local examples use 'zk2' topology. Adding Consul 0.7.2 to base docker image. etcd2topo: using lease ID in lock filenames. Implementing the Consul topology client. test: vtgate_buffer.py: Add second test for vtctl TabletExternallyReparented. test: vtgate_buffer.py: Stop the the two threads in case of a test error. test: vtgate_buffer.py: Move setup code from setUp to setUpModule. test: Add "demote_master_commands" to MysqlFlavor class. Include on clauses in impossible queries, to avoid only_full_group_by in aggregations with joins (vitessio#2433) Adds support for parsing queries with boolean value comparisons, i.e. true/false and =, !=, >, <, >=, <= (vitessio#2432) V3 Engine Code Refactor (vitessio#2423) Refactor impossible query generation, and support group by - (vitessio#2409) docker/test/run: Avoid using non-universal --tmpdir flag on mktemp (vitessio#2429) ...

MultiRow Insert Optimization

9ef1d02

googlebot added the cla: yes label Dec 30, 2016

ashudeep-sharma-zz added 2 commits December 31, 2016 00:07

Rewriting lkp.ver query using and/or clauses

4fbff44

Rewriting lkp.ver query using and/or clauses

02a3b28

sougou reviewed Jan 3, 2017

View reviewed changes

Merging Master and Removing Redundant Code

aa521ae

sougou reviewed Jan 4, 2017

View reviewed changes

ashudeep-sharma-zz added 2 commits January 5, 2017 20:00

Incorporating comments and Adding tests to improve the code-coverage

16de87b

Fixing Lint Errors

b520cc0

sougou merged commit 0d0c736 into vitessio:master Jan 5, 2017

michael-berlin mentioned this pull request Jan 6, 2017

vtgate/vindexes: Do not run testfiles.Locate() from init() (2nd time). #2438

Merged

Conversation

ashudeep-sharma-zz commented Dec 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sougou commented Jan 3, 2017

Uh oh!

ashudeep-sharma-zz commented Jan 4, 2017

Uh oh!

sougou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashudeep-sharma-zz commented Jan 5, 2017

Uh oh!

sougou commented Jan 5, 2017 • edited by alainjobart Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sougou commented Jan 5, 2017 •

edited by alainjobart

Loading