Skip to content

Add ability to hash the from value of lookup vindexes#4339

Merged
sougou merged 1 commit intovitessio:masterfrom
eeSeeGee:young.20181105.hash_from_ids
Nov 9, 2018
Merged

Add ability to hash the from value of lookup vindexes#4339
sougou merged 1 commit intovitessio:masterfrom
eeSeeGee:young.20181105.hash_from_ids

Conversation

@eeSeeGee
Copy link
Copy Markdown
Contributor

@eeSeeGee eeSeeGee commented Nov 5, 2018

Signed-off-by: Aaron Young aaron.young@gmail.com

@eeSeeGee eeSeeGee force-pushed the young.20181105.hash_from_ids branch from 27a028b to ea3f693 Compare November 9, 2018 04:22
@eeSeeGee eeSeeGee requested a review from sougou as a code owner November 9, 2018 04:22
Copy link
Copy Markdown
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

Looks like I didn't convey correctly what I meant. Basically, lookup_internal.go should be unchanged. The md5 should be done by lookup_unicodeloosemd5_hash.go. It's kind of similar to how lookup_hash.go does it, but instead, should hash the from key before sending it to lookup internal.

@eeSeeGee
Copy link
Copy Markdown
Contributor Author

eeSeeGee commented Nov 9, 2018

I see, ugly solution incentivizes refactoring it out.

Signed-off-by: Aaron Young <aaron.young@gmail.com>
@eeSeeGee eeSeeGee force-pushed the young.20181105.hash_from_ids branch from ea3f693 to e4e0d00 Compare November 9, 2018 16:12
Copy link
Copy Markdown
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

Yeah. I can totally see this spiraling out of control eventually. But it's ok for now. After a few more such offshoots, we'll need to wrangle this with better composability.

@sougou sougou merged commit f6c5cff into vitessio:master Nov 9, 2018
@eeSeeGee eeSeeGee deleted the young.20181105.hash_from_ids branch May 28, 2019 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants