-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Range Query Optimization (For sequential Vindex types) #17342
base: main
Are you sure you want to change the base?
Changes from 4 commits
0913563
40d041f
99ea8b5
e8593a8
5a7c8f1
8a0ded0
0697cb0
d2590a8
f4a578c
a46a8ab
b99ebab
1400fea
c61cd52
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4820,5 +4820,100 @@ | |
"user.authoritative" | ||
] | ||
} | ||
}, | ||
{ | ||
"comment": "Between clause on primary indexed id column (binary vindex on id)", | ||
"query": "select id from seq_tbl where id between 1 and 5", | ||
"plan": { | ||
"QueryType": "SELECT", | ||
"Original": "select id from seq_tbl where id between 1 and 5", | ||
"Instructions": { | ||
"OperatorType": "Route", | ||
"Variant": "Between", | ||
"Keyspace": { | ||
"Name": "sequential", | ||
"Sharded": true | ||
}, | ||
"FieldQuery": "select id from seq_tbl where 1 != 1", | ||
"Query": "select id from seq_tbl where id between 1 and 5", | ||
"Table": "seq_tbl", | ||
"Values": [ | ||
"(1, 5)" | ||
], | ||
"Vindex": "binary" | ||
}, | ||
"TablesUsed": [ | ||
"sequential.seq_tbl" | ||
] | ||
} | ||
}, | ||
{ | ||
"comment": "Between clause on group_id column (xxhash vindex on group_id)", | ||
"query": "select id, group_id from seq_tbl where group_id between 1 and 5", | ||
"plan": { | ||
"QueryType": "SELECT", | ||
"Original": "select id, group_id from seq_tbl where group_id between 1 and 5", | ||
"Instructions": { | ||
"OperatorType": "Route", | ||
"Variant": "Scatter", | ||
"Keyspace": { | ||
"Name": "sequential", | ||
"Sharded": true | ||
}, | ||
"FieldQuery": "select id, group_id from seq_tbl where 1 != 1", | ||
"Query": "select id, group_id from seq_tbl where group_id between 1 and 5", | ||
"Table": "seq_tbl" | ||
}, | ||
"TablesUsed": [ | ||
"sequential.seq_tbl" | ||
] | ||
} | ||
}, | ||
{ | ||
"comment": "Between clause on col2 column (there is no vindex on this column)", | ||
"query": "select id, col2 from seq_tbl where col2 between 10 and 50", | ||
"plan": { | ||
"QueryType": "SELECT", | ||
"Original": "select id, col2 from seq_tbl where col2 between 10 and 50", | ||
"Instructions": { | ||
"OperatorType": "Route", | ||
"Variant": "Scatter", | ||
"Keyspace": { | ||
"Name": "sequential", | ||
"Sharded": true | ||
}, | ||
"FieldQuery": "select id, col2 from seq_tbl where 1 != 1", | ||
"Query": "select id, col2 from seq_tbl where col2 between 10 and 50", | ||
"Table": "seq_tbl" | ||
}, | ||
"TablesUsed": [ | ||
"sequential.seq_tbl" | ||
] | ||
} | ||
}, | ||
{ | ||
"comment": "Between clause on multicolumn vindex (cola,colb)", | ||
"query": "select cola,colb,col1 from seq_multicol_tbl where cola between 1 and 5", | ||
"plan": { | ||
"QueryType": "SELECT", | ||
"Original": "select cola,colb,col1 from seq_multicol_tbl where cola between 1 and 5", | ||
"Instructions": { | ||
"OperatorType": "Route", | ||
"Variant": "Scatter", | ||
"Keyspace": { | ||
"Name": "sequential", | ||
"Sharded": true | ||
}, | ||
"FieldQuery": "select cola, colb, col1 from seq_multicol_tbl where 1 != 1", | ||
"Query": "select cola, colb, col1 from seq_multicol_tbl where cola between 1 and 5", | ||
"Table": "seq_multicol_tbl", | ||
"Values": [ | ||
"(1, 5)" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why we get the values field here for a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for catching this , noticed This ensures 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. |
||
] | ||
}, | ||
"TablesUsed": [ | ||
"sequential.seq_multicol_tbl" | ||
] | ||
} | ||
} | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -948,6 +948,61 @@ | |
] | ||
} | ||
} | ||
}, | ||
"sequential": { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure thing , incorporated with 5a7c8f1 , using |
||
"sharded": true, | ||
"vindexes": { | ||
"xxhash": { | ||
"type": "xxhash" | ||
}, | ||
"binary": { | ||
"type": "binary" | ||
}, | ||
"multicolIdx": { | ||
"type": "multiCol_test" | ||
} | ||
}, | ||
"tables": { | ||
"seq_tbl": { | ||
"column_vindexes": [ | ||
{ | ||
"column": "id", | ||
"name": "binary" | ||
}, | ||
{ | ||
"column": "group_id", | ||
"name": "xxhash" | ||
} | ||
], | ||
"columns": [ | ||
{ | ||
"name": "col1", | ||
"type": "VARCHAR" | ||
}, | ||
{ | ||
"name": "col2", | ||
"type": "INT16" | ||
} | ||
] | ||
}, | ||
"seq_multicol_tbl": { | ||
"column_vindexes": [ | ||
{ | ||
"columns": [ | ||
"cola", | ||
"colb" | ||
], | ||
"name": "multicolIdx" | ||
} | ||
], | ||
"columns" : [ | ||
{ | ||
"name": "col1", | ||
"type": "INT16" | ||
} | ||
] | ||
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -24,6 +24,7 @@ import ( | |||||||||
"reflect" | ||||||||||
"testing" | ||||||||||
|
||||||||||
"github.com/stretchr/testify/assert" | ||||||||||
"github.com/stretchr/testify/require" | ||||||||||
|
||||||||||
"vitess.io/vitess/go/sqltypes" | ||||||||||
|
@@ -145,3 +146,20 @@ func TestBinaryReverseMap(t *testing.T) { | |||||||||
t.Errorf("ReverseMap(): %v, want %s", err, wantErr) | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
// TestBinaryRangeMap takes start and env values, | ||||||||||
// and checks against a destination keyrange. | ||||||||||
func TestBinaryRangeMap(t *testing.T) { | ||||||||||
systay marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
|
||||||||||
startInterval := "0x01" | ||||||||||
endInterval := "0x10" | ||||||||||
|
||||||||||
got, err := binOnlyVindex.(Sequential).RangeMap(context.Background(), nil, sqltypes.NewHexNum([]byte(startInterval)), | ||||||||||
sqltypes.NewHexNum([]byte(endInterval))) | ||||||||||
require.NoError(t, err) | ||||||||||
want := "DestinationKeyRange(01-10)" | ||||||||||
if !assert.Equal(t, got[0].String(), want) { | ||||||||||
t.Errorf("RangeMap(): %+v, want %+v", got, want) | ||||||||||
} | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks , done with 5a7c8f1 |
||||||||||
|
||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
it'd be good to make sure that this also works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for suggesting this , added a test case now 0697cb0 , and also fixed missing Cost for
RoutOpCode.Between
.