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.

Sign up for GitHub

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

Jump to bottom

Mark non-unique lookup vindex as backfill to ignore vindex selection #14227

Merged
merged 3 commits into from
Jan 24, 2024
Merged

Mark non-unique lookup vindex as backfill to ignore vindex selection #14227

merged 3 commits into from
Jan 24, 2024

Conversation

aparajon
Copy link
Contributor

@aparajon aparajon commented Oct 10, 2023 •
edited by harshit-gangal
Loading

Description

As a follow-up to #13505, this PR enables non-unique lookups to support the isBackfilling interface. This will prevent non-unique lookups from vindex selection during a backfill (i.e. when write_only is set to true).

Related Issue(s)

Checklist

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on the CI
  • Documentation was added or is not required

Deployment Notes

@github-actions github-actions bot added this to the v19.0.0 milestone Oct 10, 2023
@aparajon aparajon force-pushed the add-backfill-support-for-nonunique-lookups branch from 4a209f2 to 67d498f Compare October 10, 2023 23:21
@aparajon aparajon marked this pull request as ready for review October 12, 2023 18:35
@github-actions GitHub Actions
Copy link
Contributor

This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:

  • Push additional commits to the associated branch.
  • Remove the stale label.
  • Add a comment indicating why it is not stale.

If no action is taken within 7 days, this PR will be closed.

@github-actions github-actions bot added the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label Nov 12, 2023
harshit-gangal
harshit-gangal reviewed Nov 14, 2023
View reviewed changes
go/vt/vtgate/vindexes/lookup.go
Comment on lines +184 to 191
// IsBackfilling implements the LookupBackfill interface
func (ln *LookupNonUnique) IsBackfilling() bool {
return ln.writeOnly
}

// Query implements the LookupPlanable interface
func (ln *LookupNonUnique) Query() (selQuery string, arguments []string) {
return ln.lkp.query()
Copy link
Member

Choose a reason for hiding this comment

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

add this to ConsistentLookup as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 7361fe245816db696c12da9306da125539aacffa, I think using *clCommon should take care of that.

@harshit-gangal harshit-gangal changed the title Add backfill support for non-unique lookups Mark non-unique lookup vindex as backfill to ignore vindex selection Nov 14, 2023
@harshit-gangal
Copy link
Member

Thank you for your contribution. I updated the title to suit the changes.

@harshit-gangal harshit-gangal added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Query Serving and removed Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. labels Nov 14, 2023
@aparajon aparajon force-pushed the add-backfill-support-for-nonunique-lookups branch 2 times, most recently from 7361fe2 to b24ce8a Compare November 20, 2023 18:05
@harshit-gangal
Copy link
Member

the plan_test is failing

  "Query": "select * from customer where name = 'x'" => "select * from customer where `name` = 'x'",

can you update the expected query so that the test can be 🟢

@aparajon aparajon force-pushed the add-backfill-support-for-nonunique-lookups branch from b24ce8a to 2152b31 Compare November 23, 2023 04:39
@aparajon
Copy link
Contributor Author

the plan_test is failing

  "Query": "select * from customer where name = 'x'" => "select * from customer where `name` = 'x'",

can you update the expected query so that the test can be 🟢

Done, would you mind running the tests again?

@frouioui
Copy link
Member

Done, would you mind running the tests again?

Done, they are re-triggered.

@GuptaManan100
Copy link
Member

Ugh, the CI failed on 2023-11-23T21:17:31.6621372Z === RUN TestFuzzRewriting/((n0_xor_n0)_and_(n1_xor_n2)_or_(n0_or_n2)_xor_(n0_xor_n1))_xor_n0 2023-11-23T21:17:31.6621491Z panic: test timed out after 10m0s. This has been fixed in main. Would you mind merging main so that the tests can go green

@aparajon aparajon force-pushed the add-backfill-support-for-nonunique-lookups branch from 2152b31 to 021d6bb Compare December 6, 2023 02:52
@aparajon
Copy link
Contributor Author

aparajon commented Dec 6, 2023

Ugh, the CI failed on 2023-11-23T21:17:31.6621372Z === RUN TestFuzzRewriting/((n0_xor_n0)_and_(n1_xor_n2)_or_(n0_or_n2)_xor_(n0_xor_n1))_xor_n0 2023-11-23T21:17:31.6621491Z panic: test timed out after 10m0s. This has been fixed in main. Would you mind merging main so that the tests can go green

Sure, just did. Is there a way to enable CI to run after a new git change is made in this PR? Don't want to keep bothering you all 😄

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 6, 2024

This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:

  • Push additional commits to the associated branch.
  • Remove the stale label.
  • Add a comment indicating why it is not stale.

If no action is taken within 7 days, this PR will be closed.

@github-actions github-actions bot added the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label Jan 6, 2024
@systay
Copy link
Collaborator

systay commented Jan 8, 2024

hi @aparajon

not sure why CI is not starting. if you merge main again and push, it should start, and then we can merge your contribution.

sorry for the hassle

@github-actions github-actions bot removed the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label Jan 9, 2024
@aparajon
Add backfill support for non-unique lookups
7da5132 
Signed-off-by: Armand Parajon <armand@squareup.com>
@aparajon
Make ConsistentLookup implement IsBackfilling
110f0c9 
Signed-off-by: Armand Parajon <armand@squareup.com>
@aparajon
Fix query format
7fd1740 
Signed-off-by: Armand Parajon <armand@squareup.com>
@aparajon aparajon force-pushed the add-backfill-support-for-nonunique-lookups branch from 021d6bb to 7fd1740 Compare January 9, 2024 06:56
@aparajon
Copy link
Contributor Author

aparajon commented Jan 9, 2024

hi @aparajon

not sure why CI is not starting. if you merge main again and push, it should start, and then we can merge your contribution.

sorry for the hassle

Thanks @systay, I wonder if it's due to the way I've been syncing updates with main. I've been doing:

  1. Sync my fork with main through the UI
  2. Run git pull --rebase in my fork
  3. Run git push origin -f add-backfill-support-for-nonunique-lookups in my fork

Just ran this again.

@frouioui frouioui merged commit 58bb702 into vitessio:main Jan 24, 2024
99 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harshit-gangal harshit-gangal harshit-gangal approved these changes

@frouioui frouioui frouioui approved these changes

@GuptaManan100 GuptaManan100 GuptaManan100 approved these changes

@systay systay Awaiting requested review from systay systay is a code owner

@arthurschreiber arthurschreiber Awaiting requested review from arthurschreiber arthurschreiber is a code owner

Assignees
No one assigned
Labels
Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Milestone
v19.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@aparajon @harshit-gangal @frouioui @GuptaManan100 @systay