Skip to content
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

v3: support overlapping multi-column vindexes #4996

Merged
merged 1 commit into from
Aug 24, 2019

Conversation

sougou
Copy link
Contributor

@sougou sougou commented Jul 15, 2019

There exist use cases where one needs vindexes with overlapping columns. In such scenarios, the extraction of values cannot be done simultaneously with the substitution of bindvars. If so, the first vindex will substitute its bindvar name to replace an existing value, and then a subsequent vindex may pick up the previously substituted value instead of the original value.

With this logic change, we pull all necessary values out of the insert values in the first pass, and substitutes those values with bind var names in a separate pass.

Signed-off-by: Sugu Sougoumarane ssougou@gmail.com

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
@sougou sougou requested a review from systay July 15, 2019 04:43
@sougou
Copy link
Contributor Author

sougou commented Jul 15, 2019

@eeSeeGee

@eeSeeGee
Copy link
Contributor

Does this need a specific test for when vindex A is write_only vs vindex B? i.e. one thing somebody might actually do with this?

@sougou
Copy link
Contributor Author

sougou commented Jul 15, 2019

Does this need a specific test for when vindex A is write_only vs vindex B? i.e. one thing somebody might actually do with this?

I believe not. This fix is orthogonal of whether the vindex is RW or write-only.

@sougou sougou requested a review from deepthi July 27, 2019 18:15
@sougou
Copy link
Contributor Author

sougou commented Aug 17, 2019

@eeSeeGee ping

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

LGTM

"Sharded": true
},
"Query": "insert into overlap_vindex(kid, column_a, column_b) values (:_kid0, :_column_a0, 3)",
"Values": [
Copy link
Member

Choose a reason for hiding this comment

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

this would be easier to understand if we called this VindexValues while converting to JSON just like in the original Insert struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Makes sense. We should make a separate PR.

@sougou sougou merged commit 6467c98 into vitessio:master Aug 24, 2019
@sougou sougou deleted the ss-multi-lookup branch August 24, 2019 19:29
systay pushed a commit that referenced this pull request Jul 22, 2024
Signed-off-by: Manan Gupta <manan@planetscale.com>
Co-authored-by: Manan Gupta <35839558+GuptaManan100@users.noreply.github.com>
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.

3 participants