-
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
Introduce Concatenated Fixed-width Composite or CFC vindex #7537
Conversation
Reviewers, please see also the discussion on #7332 |
1908d6c
to
4cf9b98
Compare
Regarding the license header, you can use this one:
|
|
||
// Cost returns the cost as 1. | ||
func (vind *CFC) Cost() int { | ||
return 1 |
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.
Did you consider other vindex costs when you arrived at this value?
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.
I'm not so familiar the numeric system to infer the vindex cost. Do you have suggestions for how to calculate the cost ?
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.
The cost can be equal to number of columns involved.
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.
can you change the cost to sum of cost of hash function involved for each column
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.
which is no. of column * cost of hash function
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.
when offsets
parameter is set, we know the number of columns but if it isn't set, we have no idea how many columns are involved. What do you suggest in this case ?
I had only minor nitpicks to contribute with, except from maybe wanting to avoid the type casting in |
// to a `key.DestinationKeyRange`. Note that the prefix to key range mapping is | ||
// only active in 'LIKE' expression. When a column with CFC defined appears in | ||
// other expressions, e.g. =, !=, IN etc, it behaves exactly as other | ||
// functional unique vindexes. |
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.
Since internally keyspace ID is a byte array, it's not immediately clear what would be on the righthand side of the comparison operator. It could be a byte sequence, or a hexadecimal string, or some other representation.
I see from your tests that you use hexadecimal. That makes sense, since that's how they are represented in public-facing documentation, and also in the in_keyrange()
function, and also in shard names.
You should make that explicit in the documentation here, and anywhere else relevant.
EDIT: Reading the code more, I see you are not using hexadecimal (but that wasn't immediately clear, because all of your test cases only used the characters [a-fA-F0-9]). You are taking whatever is on the right-hand side and interpreting it as raw bytes.
|
||
// we don't use the full hashed value because it's very long. | ||
// keyrange resolution is done via comparing []byte so longer | ||
// keyspace ids have performance impact. |
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.
If you use hashing functions that are already in use for vindexes, then that shouldn't be an issue, right? If they were already too long, then they would already be causing issues.
Maybe smaller, more performant hash functions are needed just for cases where you are concatenating more than 3 values together, but for the common use-cases of 2 or 3 values, maybe an ordinary vindex hash function, in its entirety, would do?
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.
md5 hash is 16-byte. If there are only 2 or 3 components in the composite index, you are right that it doesn't matter much. However, the interface here allows arbitrarily concatenated composite index. Hence this comment is about this generic case. Specifically it makes the hashed value (of the concatenation) at most same size as the input value so that there is no added performance cost in bytes.Compare
if people choose to use many components in the composite index.
func md5hash(in []byte) []byte { | ||
n := len(in) | ||
out := vMD5Hash(in) | ||
if n < len(out) { |
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.
Based on your current implementation, this means that your unhashed data is coupled to the hashed data size. I cannot choose to take an 8-byte value, and hash it to a 4-byte value.
Unless you do all the hashing yourself, and leave hash
and offsets
defined, which I think is perfectly fine.
I'm more pointing out that, I think this is going to limit the utility of the hashing.
If I take my 8-byte value, choose 4 of the bytes, and then hash that, the composite hash might not be distributed as well, if I'm wrong about the distribution of those 4 bytes.
Though, the approach you're taking now has similar issues. If the hashing function returns an N-byte array, and you only take the first M-bytes of that array, it might not be distributed very well. If you really want an M-byte array instead, you should choose a function that is designed to return M-bytes, while still acting as a good hash function.
All of this makes me question whether doing automatic hashing is worth it. Or if the client should compute their own hashes, that way they can make their own determination how good of a hash they need.
The other limiting requirement here is the fact that the inputs must be fixed-width. With the client defining the function that produces the column value, they can support variable-width component functions, e.g. identity(c1) concat identity(c2)
, or reverse(c1) concat reverse(c2)
. You don't have that kind of flexibility with this hashing system. Which isn't necessarily a bad thing. But again, I'm wondering how much these hashes would end up being used, versus manual value mapping in the client.
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.
I think it would help to see some real-world examples of how you are planning on using this. The RFC and tests only contain toy schemas. What would it look like in a production schema, which wanted to achieve soft-colocation using this CFC concept.
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 the comments. I debated with myself for whether to support hashing at all, very similar to what you said. Finally I decided to add it as a way of convenience so that application doesn't have to do it. At the end of the day, there are only a limited number of shards, regardless how we hash it it's just a best effort to distribute the keyspace ids. When the components in a composite index have some narrow value range, e.g. some column in the composite index may be an enum, adding a parameter to vschema is arguably more convenient than changing application code to produce a hashed concatenated column.
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.
A more concrete example: inventory system. A simplified data model would be that, there is 'product' within which there are many items. The mental model is, there could be couple orders of magnitude more items than products. Items check-in and check-out the inventory for sales/return/restock etc. We want to have per product view of its inventory status, e.g. how many items in stock within an hour block, average time in the warehouse in the last 7 days, return rate, etc.
product {
product_id
product_name
msrp
}
item {
item_id
product_id
serial_no
price
check_in_at
check_out_at
status
}
Let's assume the status change or item creation has very high throughput. We can shard by item id and create a global secondary index on product id and normalize all the columns needed in the query with the index. But we have to perform two inserts to insert one item, one in the item table the other in the global secondary index table on another shard. It has its pros and cons. CFC vindex provides another option. We can have the item table look like
item {
item_id
product_id
serial_no
price
check_in_at
check_out_at
status
keyid // first 2 bytes of product_id + item_id
}
If we define CFC vindex on keyid
, given a product_id, we can then use
select count(*) from items where keyid like 'xx%' and product_id = 'xxxxxx' and check_in_at > t0 and check_in_at <= t1 and check_out_at is null
to find out within (t0, t1) how many items for the product whose id is 'xxxxxx'. This query only hit a subset of shards instead of all shards. (i.e. 'xx123' and 'xx456' may very well belong to two different shards) The write throughput of inserting/updating an item is spread across more than one shard so that its qps is not bottlenecked by a single shard.
a5d9995
to
dfc99ea
Compare
|
||
// Cost returns the cost as 1. | ||
func (vind *CFC) Cost() int { | ||
return 1 |
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.
The cost can be equal to number of columns involved.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
61aaf7c
to
8d270a5
Compare
Anyone knows how to fix this test which I didn't touch ? @systay @harshit-gangal
|
Signed-off-by: Meng Wang <[email protected]>
Signed-off-by: Meng Wang <[email protected]>
@systay @harshit-gangal PTAL |
I see there are still few comments which are not addressed. I have some additional comments as well. Also, for clarity you can make the comment as resolved if you have replied or changed the code accordingly. |
Signed-off-by: Meng Wang <[email protected]>
Signed-off-by: Meng Wang <[email protected]>
@harshit-gangal PTAL |
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.
LGTM.
Just have one comment.
Signed-off-by: Meng Wang <[email protected]>
@wangmeng99 Could you add the documentation regarding cfc index here: https://vitess.io/docs/reference/features/vindexes/ |
Description
The purpose of this vindex is to shard the rows based on the prefix of
sharding key. Imagine the sharding key is defined as (s1, s2, ... sN), a
prefix of this key is (s1, s2, ... sj) (j <= N). This vindex puts the rows
with the same prefix among a same group of shards instead of scatter them
around all the shards. The benefit of doing so is that prefix queries will
only fanout to a subset of shards instead of all the shards. Specifically
this vindex maps the full key, i.e. (s1, s2, ... sN) to a
key.DestinationKeyspaceID
and the prefix of it, i.e. (s1, s2, ... sj)(j<N)to a
key.DestinationKeyRange
. Note that the prefix to key range mapping isonly active in 'LIKE' expression. When a column with CFC defined appears in
other expressions, e.g. =, !=, IN etc, it behaves exactly as other
functional unique vindexes.
Related Issue(s)
Checklist
Deployment Notes
Impacted Areas in Vitess
Components that this PR will affect: