Range Query Optimization (For sequential Vindex types) #17342

c-r-dev · 2024-12-06T19:57:00Z

Description

Queries that specify values in a range (for vindexed column) are sent to all shards, instead of just the shards which contain the values in the range.

For primary vindexs which are sequential and not random, a BINARY vindex for example, optimized range queries at vtgate to be sent to only shards included between the first and last keyspaceid values of the range.

Related Issue(s)

Fixes #9808

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

Made the change for sequential Vindexes ( binary and numeric)

Tests Performed

Added unit tests module level
Included sample schema with two columns (c1 and c2) , c1 is vindexed (type binday) and c2 is not vindexed.

When range queried on column c1 (vindex column) :

bin % ./vtexplain --shards 4 --vschema-file ../examples/vtexplain/sample_vschema.json --schema-file ../examples/vtexplain/sample_schema.sql --sql "select * from t1 where c1 between 1 and 10"

----------------------------------------------------------------------
select * from t1 where c1 between 1 and 10

1 ks1/-40: select * from t1 where c1 between 1 and 10 limit 10001

----------------------------------------------------------------------
<nil>%

When range queried on column c2 (non vindex column) :

bin % ./vtexplain --shards 4 --vschema-file ../examples/vtexplain/sample_vschema.json --schema-file ../examples/vtexplain/sample_schema.sql --sql "select * from t1 where c2 between 1 and 10"

----------------------------------------------------------------------
select * from t1 where c2 between 1 and 10

1 ks1/-40: select * from t1 where c2 between 1 and 10 limit 10001
1 ks1/40-80: select * from t1 where c2 between 1 and 10 limit 10001
1 ks1/80-c0: select * from t1 where c2 between 1 and 10 limit 10001
1 ks1/c0-: select * from t1 where c2 between 1 and 10 limit 10001

----------------------------------------------------------------------
<nil>%

Signed-off-by: c-r-dev <[email protected]>

vitess-bot · 2024-12-06T19:57:03Z

deepthi · 2024-12-08T18:34:30Z

I unchecked this, because CI has not yet run

Did the new or modified tests pass consistently locally and on CI?

c-r-dev · 2024-12-09T13:55:13Z

I unchecked this, because CI has not yet run

Did the new or modified tests pass consistently locally and on CI?

thanks, confirm - modified tests passed consistently locally ( i have run module level unit tests )

vitess.io/vitess/go/vt/key
vitess.io/vitess/go/vt/vtgate/engine
vitess.io/vitess/go/vt/vtgate/vindexes
vitess.io/vitess/go/vt/vtgate/planbuilder/operators

Didn't get CI to run locally end to end. Currently working on the CI setup.

systay

Thank you so much for your contribution!
This looks really good from my POV.

I would love to see a plan test and and one end-to-end test as well. Not sure where the end-to-end test should live. @harshit-gangal probably knows.

go/vt/vtgate/planbuilder/operators/sharded_routing.go

go/vt/vtgate/vindexes/binary_test.go

harshit-gangal

We should add plan test that uses Between for Sequential Vindex and Non-Sequential Vindex type

It can be added to go/vt/vtgate/planbuilder/testdata/select_cases.json

go/vt/vtgate/vindexes/vindex.go

go/vt/vtgate/planbuilder/operators/sharded_routing.go

examples/vtexplain/sample_vschema.json

go/vt/vtgate/engine/routing.go

go/vt/vtgate/planbuilder/operators/sharded_routing.go

c-r-dev · 2024-12-10T15:15:50Z

thanks @systay and @harshit-gangal for reviewing and sharing feedback, let me incorporate these and get back. Will reach out in case of questions.

… feedback Signed-off-by: c-r-dev <[email protected]>

Signed-off-by: c-r-dev <[email protected]>

…ema files) Signed-off-by: c-r-dev <[email protected]>

c-r-dev · 2024-12-11T01:47:00Z

We should add plan test that uses Between for Sequential Vindex and Non-Sequential Vindex type

It can be added to go/vt/vtgate/planbuilder/testdata/select_cases.json

Thanks for the suggestion and pointing me to sample , i have added new test cases with e8593a8

systay · 2024-12-11T08:14:21Z

go/vt/vtgate/planbuilder/testdata/vschemas/schema.json

@@ -948,6 +948,61 @@
          ]
        }
      }
+    },
+    "sequential": {


not sure, but I'm thinking we could use an existing keyspace instead of adding a new one. feel free to add a vindex or even a table if you need to.
I'm just thinking of not adding unnecessary complexity if we don't have to

sure thing , incorporated with 5a7c8f1 , using user keyspace - added extra vindex and table

systay · 2024-12-11T08:16:37Z

go/vt/vtgate/planbuilder/testdata/filter_cases.json

+      ]
+    }
+},
+{


nice set of planner tests. could you add at least one test where the values in the between are not literal values, but instead columns from another table. something like:

select * from tblA join tblB on tblA.foo = tbl.bar where tblA.X between tblB.C and tblB.D

it'd be good to make sure that this also works as expected.

thanks for suggesting this , added a test case now 0697cb0 , and also fixed missing Cost for RoutOpCode.Between.

systay · 2024-12-11T08:18:12Z

go/vt/vtgate/vindexes/binary_test.go

+	if !assert.Equal(t, got[0].String(), want) {
+		t.Errorf("RangeMap(): %+v, want %+v", got, want)
+	}


Suggested change

if !assert.Equal(t, got[0].String(), want) {

t.Errorf("RangeMap(): %+v, want %+v", got, want)

}

assert.Equal(t, want, got[0].String())

thanks , done with 5a7c8f1

systay · 2024-12-11T08:23:02Z

go/vt/vtgate/planbuilder/testdata/filter_cases.json

+      "Query": "select cola, colb, col1 from seq_multicol_tbl where cola between 1 and 5",
+      "Table": "seq_multicol_tbl",
+      "Values": [
+        "(1, 5)"


I don't understand why we get the values field here for a Scatter route. it doesn't look right

thanks for catching this , noticed processSingleColumnVindex has few extra early checks compared to processMultiColumnVindex. With 8a0ded0 , added checks for vindex and also routingCode before processing any futher.

This ensures Values is not set in case of Scatter route.

Please help to review if addition of these checks has any other side effects (which i may have missed) , ensured all test cases are passing.

c-r-dev · 2024-12-11T12:36:18Z

thanks @systay , will check these and make next set of changes.

Signed-off-by: c-r-dev <[email protected]>

…ar to processSingleColumnVindex) Signed-off-by: c-r-dev <[email protected]>

Signed-off-by: c-r-dev <[email protected]>

codecov · 2024-12-12T12:02:27Z

Codecov Report

Attention: Patch coverage is 44.70588% with 47 lines in your changes missing coverage. Please review.

Project coverage is 67.51%. Comparing base (45192d2) to head (1400fea).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/vtgate/engine/routing.go	0.00%	30 Missing ⚠️
go/vt/vtgate/vindexes/numeric.go	0.00%	11 Missing ⚠️
go/vt/vtgate/vindexes/binary.go	63.63%	4 Missing ⚠️
...vt/vtgate/planbuilder/operators/sharded_routing.go	93.54%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17342      +/-   ##
==========================================
- Coverage   67.52%   67.51%   -0.01%     
==========================================
  Files        1581     1581              
  Lines      253948   254030      +82     
==========================================
+ Hits       171480   171520      +40     
- Misses      82468    82510      +42

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

harshit-gangal

Changes looks good.
For end to end test. We recently added a way to test the plan tests to also run as e2e test

Look here https://github.com/vitessio/vitess/blob/main/go/test/endtoend/vtgate/plan_tests/plan_e2e_test.go
and the PR that added it #17117

You would have to add filter_cases.json to the e2e test.

c-r-dev · 2024-12-13T13:33:07Z

Changes looks good. For end to end test. We recently added a way to test the plan tests to also run as e2e test

Look here https://github.com/vitessio/vitess/blob/main/go/test/endtoend/vtgate/plan_tests/plan_e2e_test.go and the PR that added it #17117

You would have to add filter_cases.json to the e2e test.

thanks @harshit-gangal , will pull in the changes from #17117 and add test case for filer_case.json. Will reach out in case of any questions.

Signed-off-by: c-r-dev <[email protected]>

c-r-dev · 2024-12-14T04:12:40Z

Changes looks good. For end to end test. We recently added a way to test the plan tests to also run as e2e test
Look here https://github.com/vitessio/vitess/blob/main/go/test/endtoend/vtgate/plan_tests/plan_e2e_test.go and the PR that added it #17117
You would have to add filter_cases.json to the e2e test.

thanks @harshit-gangal , will pull in the changes from #17117 and add test case for filter_case.json. Will reach out in case of any questions.

Thanks , added end-to-end test case for filter_case.json with f4a578c. Please help to review if skip_e2e is tagged correctly ( kept it similar to select_case.json - join cases (no sample data in table) and known error cases )

go/vt/vtgate/planbuilder/testdata/filter_cases.json

harshit-gangal · 2024-12-16T05:45:05Z

go/vt/vtgate/planbuilder/testdata/filter_cases.json

+  },
+  "skip_e2e":true
+}


new test should not be skipped

thanks , out of the 5 new test cases added for between , marked the final one related to join as skip_e2e.

Reason for marking this skip , is because of "OperatorType": "Join" , verifyTestExpectations for join cases verifies -> Verify that the Join primitive sees atleast 1 row on the left side. Currently we don't have any sample data in these test schema tables. (pls let me know if we can put some test data in these tables for e2e testing)

@harshit-gangal please help to let me know if I have missed any thing , or if there is a better way to write this test for "Between clause on a binary vindex field with values from a different table"

We need to add the sample data to have data on left and right side of the join.
The policy after adding these plan test is that we need to ensure that new tests does not add skip_e2e flag.
For that we have to keep modifying the sample input data.

thanks @harshit-gangal , added sample data loading with 1400fea , and removed skip_e2e. All new tests are passing with out skipping.

Please help to review.

c-r-dev · 2024-12-16T13:16:35Z

thanks @harshit-gangal for reviewing , will incorporate feedback and share and updated version for review.

Signed-off-by: c-r-dev <[email protected]>

c-r-dev · 2024-12-18T15:00:51Z

Thank you so much for your contribution! This looks really good from my POV.

I would love to see a plan test and and one end-to-end test as well. Not sure where the end-to-end test should live. @harshit-gangal probably knows.

@systay , thanks for the feedback , have worked with @harshit-gangal and got end-to-end setup for filter_case.

Can you please help to review when you get a chance.

added a new between vindex interface

0913563

Signed-off-by: c-r-dev <[email protected]>

github-actions bot added this to the v22.0.0 milestone Dec 6, 2024

deepthi marked this pull request as ready for review December 9, 2024 17:31

deepthi requested review from harshit-gangal, systay, frouioui, GuptaManan100, arthurschreiber and deepthi as code owners December 9, 2024 17:31

systay requested changes Dec 10, 2024

View reviewed changes

go/vt/vtgate/planbuilder/operators/sharded_routing.go Outdated Show resolved Hide resolved

go/vt/vtgate/vindexes/binary_test.go Show resolved Hide resolved

harshit-gangal requested changes Dec 10, 2024

View reviewed changes

harshit-gangal reviewed Dec 10, 2024

View reviewed changes

go/vt/vtgate/planbuilder/operators/sharded_routing.go Outdated Show resolved Hide resolved

c-r-dev added 3 commits December 10, 2024 16:18

renamed between interface to sequential and incorporated other review…

40d041f

… feedback Signed-off-by: c-r-dev <[email protected]>

planBetweenOp to check if vindex is of Sequential type

99ea8b5

Signed-off-by: c-r-dev <[email protected]>

added plan_test for between clause (and cleaned up unused example sch…

e8593a8

…ema files) Signed-off-by: c-r-dev <[email protected]>

c-r-dev requested review from harshit-gangal and systay December 11, 2024 01:49

systay reviewed Dec 11, 2024

View reviewed changes

c-r-dev added 3 commits December 11, 2024 09:54

modified test cases and reused existing test schema user

5a7c8f1

Signed-off-by: c-r-dev <[email protected]>

added early checks while evaluating matching multicolumnvindex (simil…

8a0ded0

…ar to processSingleColumnVindex) Signed-off-by: c-r-dev <[email protected]>

add missing Cost for RoutOpCode Between and corresponding test case )

0697cb0

Signed-off-by: c-r-dev <[email protected]>

c-r-dev requested a review from systay December 11, 2024 16:54

harshit-gangal reviewed Dec 13, 2024

View reviewed changes

c-r-dev added 2 commits December 13, 2024 14:04

Merge branch 'vitessio:main' into query-serving_range_query

d2590a8

added end-to-end testcase support for filter_cases

f4a578c

Signed-off-by: c-r-dev <[email protected]>

c-r-dev requested a review from harshit-gangal December 14, 2024 04:15

harshit-gangal reviewed Dec 16, 2024

View reviewed changes

c-r-dev added 2 commits December 16, 2024 14:57

Merge branch 'vitessio:main' into query-serving_range_query

a46a8ab

restored existing filter_cases test cases

b99ebab

Signed-off-by: c-r-dev <[email protected]>

c-r-dev requested a review from harshit-gangal December 16, 2024 21:17

added sample test data for filter_cases e2e testing

1400fea

Signed-off-by: c-r-dev <[email protected]>

harshit-gangal approved these changes Dec 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Range Query Optimization (For sequential Vindex types) #17342

Range Query Optimization (For sequential Vindex types) #17342

c-r-dev commented Dec 6, 2024 •

edited by deepthi

Loading

vitess-bot bot commented Dec 6, 2024

deepthi commented Dec 8, 2024

c-r-dev commented Dec 9, 2024

systay left a comment

harshit-gangal left a comment

c-r-dev commented Dec 10, 2024

c-r-dev commented Dec 11, 2024

systay Dec 11, 2024

c-r-dev Dec 11, 2024

systay Dec 11, 2024

c-r-dev Dec 11, 2024

systay Dec 11, 2024

c-r-dev Dec 11, 2024

systay Dec 11, 2024

c-r-dev Dec 11, 2024

c-r-dev commented Dec 11, 2024

codecov bot commented Dec 12, 2024 •

edited

Loading

harshit-gangal left a comment

c-r-dev commented Dec 13, 2024

c-r-dev commented Dec 14, 2024 •

edited

Loading

harshit-gangal Dec 16, 2024

c-r-dev Dec 16, 2024

harshit-gangal Dec 17, 2024

c-r-dev Dec 17, 2024 •

edited

Loading

c-r-dev commented Dec 16, 2024

c-r-dev commented Dec 18, 2024

Range Query Optimization (For sequential Vindex types) #17342

Are you sure you want to change the base?

Range Query Optimization (For sequential Vindex types) #17342

Conversation

c-r-dev commented Dec 6, 2024 • edited by deepthi Loading

Description

Related Issue(s)

Checklist

Deployment Notes

Tests Performed

vitess-bot bot commented Dec 6, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

deepthi commented Dec 8, 2024

c-r-dev commented Dec 9, 2024

systay left a comment

Choose a reason for hiding this comment

harshit-gangal left a comment

Choose a reason for hiding this comment

c-r-dev commented Dec 10, 2024

c-r-dev commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-r-dev commented Dec 11, 2024

codecov bot commented Dec 12, 2024 • edited Loading

Codecov Report

harshit-gangal left a comment

Choose a reason for hiding this comment

c-r-dev commented Dec 13, 2024

c-r-dev commented Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-r-dev Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

c-r-dev commented Dec 16, 2024

c-r-dev commented Dec 18, 2024

c-r-dev commented Dec 6, 2024 •

edited by deepthi

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading

c-r-dev commented Dec 14, 2024 •

edited

Loading

c-r-dev Dec 17, 2024 •

edited

Loading