Skip to content

VStreamer: change in filter logic#18319

Merged
rohit-nayak-ps merged 9 commits intovitessio:mainfrom
planetscale:rohit/filter-rows-no-partial
Jul 8, 2025
Merged

VStreamer: change in filter logic#18319
rohit-nayak-ps merged 9 commits intovitessio:mainfrom
planetscale:rohit/filter-rows-no-partial

Conversation

@rohit-nayak-ps
Copy link
Member

@rohit-nayak-ps rohit-nayak-ps commented Jun 3, 2025 •
edited
Loading

Description

⚠️ This is currently an experimental change. It is a breaking change in Vitess, so we need to evaluate any failing tests that need to be modified for this logic and validate that we are not breaking core Vitess workflows.

This PR changes the filter logic in vstreamer filters to send both Before and After images, if filter matches either for non-sharded workflows. Currently only the image which passes is sent, causing downstream consumers to treat the event incorrectly as an insert or delete, when it is actually an update.

For sharded workflows, the current behaviour continues, to support rows that migrate from one shard to another.

⚠️ Breaking Change
The following tests had to be updated: TestFilteredInt and TestFilteredVarBinary. The changes were to add both after/before images where the previous implementation would result in only one of the before/after images to be sent in the RowEvent.

This will not impact MoveTables or Reshard or any flows where the columns participating in the filter does not change.

Related Issue(s)

Fixes #18426

Checklist

  • "Backport to:" labels have been added if this change should be back-ported to release branches
  • If this change is to be back-ported to previous releases, a justification is included in the PR description
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on CI?
  • Documentation was added or is not required

Deployment Notes

@rohit-nayak-ps rohit-nayak-ps self-assigned this Jun 3, 2025
@rohit-nayak-ps rohit-nayak-ps added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: VReplication labels Jun 3, 2025
@vitess-bot
Copy link
Contributor

vitess-bot bot commented Jun 3, 2025

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

  • Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

  • Apply the release notes (needs details) label if users need to know about this change.
  • New features should be documented.
  • There should be some code comments as to why things are implemented the way they are.
  • There should be a comment at the top of each new or modified test to explain what the test does.

New flags

  • Is this flag really necessary?
  • Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
  • vtctl command output order should be stable and awk-able.

@vitess-bot vitess-bot bot added NeedsBackportReason If backport labels have been applied to a PR, a justification is required NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Jun 3, 2025
@github-actions github-actions bot added this to the v23.0.0 milestone Jun 3, 2025
@rohit-nayak-ps rohit-nayak-ps requested a review from deepthi June 3, 2025 20:05
@rohit-nayak-ps rohit-nayak-ps removed NeedsWebsiteDocsUpdate What it says NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Jun 3, 2025
@rohit-nayak-ps rohit-nayak-ps force-pushed the rohit/filter-rows-no-partial branch from 9f173e1 to 21be3a6 Compare June 4, 2025 07:59
@codecov
Copy link

codecov bot commented Jun 4, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 73.80952% with 22 lines in your changes missing coverage. Please review.

Project coverage is 67.50%. Comparing base (b111270) to head (368d7ec).
Report is 35 commits behind head on main.

Files with missing lines Patch % Lines
go/vt/vttablet/tabletserver/vstreamer/vstreamer.go 75.00% 13 Missing ⚠️
.../vt/vttablet/tabletserver/vstreamer/planbuilder.go 69.23% 8 Missing ⚠️
.../vt/vttablet/tabletserver/vstreamer/rowstreamer.go 83.33% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #18319      +/-   ##
==========================================
+ Coverage   67.49%   67.50%   +0.01%     
==========================================
  Files        1603     1607       +4     
  Lines      262426   262736     +310     
==========================================
+ Hits       177112   177368     +256     
- Misses      85314    85368      +54

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rohit-nayak-ps rohit-nayak-ps force-pushed the rohit/filter-rows-no-partial branch from 21be3a6 to 5339a1e Compare June 4, 2025 09:21
@rohit-nayak-ps
Add failing test
5ef08c0 
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps
Working (new) unit test
4709fc0 
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps
Return only the image which passes the filter if we are also sharding…
8e69046 
…. Some refactor. Fix TestFilteredInt for new behaviour

Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps
Fix rowstreamer to properly copy rows
a33621e 
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps
Fix TestFilteredVarBinary
7e14681 
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps rohit-nayak-ps force-pushed the rohit/filter-rows-no-partial branch from 5339a1e to 7e14681 Compare June 4, 2025 18:11
@rohit-nayak-ps
Enhance new unit test
71601ef 
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps
Remove debug comments
a0b60c0 
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps
Refactor
c9a8605 
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps rohit-nayak-ps changed the title [Work In Progress] VStreamer: change in filter logic VStreamer: change in filter logic Jun 4, 2025
deepthi
deepthi approved these changes Jun 5, 2025
View reviewed changes
go/vt/vttablet/tabletserver/vstreamer/planbuilder.go
// It returns:
// - bool: true if the row should be included in the stream (passes all filters)
// - bool: true if a vindex filter was applied (indicates sharded filtering)
func (plan *Plan) shouldFilter(values []sqltypes.Value, charsets []collations.ID) (bool, bool, error) {
Copy link
Collaborator

@deepthi deepthi Jun 5, 2025

Choose a reason for hiding this comment

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

For readability it will be good to name the return variables.

@rohit-nayak-ps rohit-nayak-ps marked this pull request as ready for review June 6, 2025 08:27
beingnoble03
beingnoble03 approved these changes Jul 7, 2025
View reviewed changes
Copy link
Member

@beingnoble03 beingnoble03 left a comment

Choose a reason for hiding this comment

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

lgtm

@rohit-nayak-ps rohit-nayak-ps removed the NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work label Jul 7, 2025
@rohit-nayak-ps
Address review comment
368d7ec 
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
@rohit-nayak-ps rohit-nayak-ps removed the NeedsIssue A linked issue is missing for this Pull Request label Jul 7, 2025
@rohit-nayak-ps rohit-nayak-ps merged commit d9380c1 into vitessio:main Jul 8, 2025
104 of 111 checks passed
@rohit-nayak-ps rohit-nayak-ps deleted the rohit/filter-rows-no-partial branch July 8, 2025 07:37
morgo added a commit to morgo/vitess that referenced this pull request Jul 21, 2025
@morgo
Merge remote-tracking branch 'origin/master' into mysql-topo
f49a6c2 
* origin/master:
  bugfix: Fix impossible query for UNION (vitessio#18463)
  fix topo use in local_example (vitessio#18357)
  fix: update go-upgrade tool to check patch number (vitessio#18252) (vitessio#18402)
  Update MAINTAINERS.md and CODEOWNERS (vitessio#18462)
  Add logging to binlog watcher actions (vitessio#18264)
  `schemadiff`: `RelatedForeignKeyTables()` (vitessio#18195)
  `vtorc`: allow recoveries to be disabled from startup (vitessio#18005)
  Fix `vttablet` not being marked as not serving when MySQL stalls (vitessio#17883)
  make xtrabackup ShouldDrainForBackup configurable (vitessio#18431)
  Reset in-memory sequence info on vttablet on UpdateSequenceTables request (vitessio#18415)
  Fix watcher storm during topo outages (vitessio#18434)
  Online DDL: resume vreplication after cut-over/RENAME failure (vitessio#18428)
  Online DDL cutover enhancements (vitessio#18423)
  VStreamer: change in filter logic (vitessio#18319)
  Online DDL metrics: `OnlineDDLStaleMigrationMinutes` (vitessio#18417)

Signed-off-by: Morgan Tocker <tocker@gmail.com>
This was referenced Jul 27, 2025
Closed
Closed
@systay systay mentioned this pull request Aug 29, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shlomi-noach shlomi-noach shlomi-noach approved these changes

@beingnoble03 beingnoble03 beingnoble03 approved these changes

@harshit-gangal harshit-gangal Awaiting requested review from harshit-gangal harshit-gangal is a code owner

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

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

+1 more reviewer

@deepthi deepthi deepthi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rohit-nayak-ps rohit-nayak-ps

Labels

Component: VReplication Type: Enhancement Logical improvement (somewhere between a bug and feature)

Projects

None yet

Milestone

v23.0.0

Development

Successfully merging this pull request may close these issues.

VStream: if a filter matches only one of before or after images, only the matching image is sent

4 participants

@rohit-nayak-ps @deepthi @shlomi-noach @beingnoble03