Skip to content

Conversation

systay
Copy link
Collaborator

@systay systay commented May 9, 2025

Description

This PR restores subquery merging behavior that was unintentionally broken by #11379, undoing the regression that reintroduced suboptimal query planning previously addressed in #10966.

Background

Given a query like:

SELECT music.id
FROM music
WHERE music.id IN (
  SELECT MAX(music.id)
  FROM music
  WHERE music.user_id IN (5, 6)
  GROUP BY music.user_id
)

We expect the planner to merge the subquery and send at most one query per shard based on the music.user_id IN (5, 6) condition. Currently, however, the planner fails to do this and ends up sending multiple queries per shard.

Curiously, wrapping the subquery in a derived table works as expected:

SELECT music.id
FROM music
WHERE music.id IN (
  SELECT * FROM (
    SELECT MAX(music.id)
    FROM music
    WHERE music.user_id IN (5, 6)
    GROUP BY music.user_id
  ) AS _inner
)

This PR restores the merging behavior so that both forms behave consistently and optimally.

Related Issue(s)

Fixes #18259

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

Copy link
Contributor

vitess-bot bot commented May 9, 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 May 9, 2025
@github-actions github-actions bot added this to the v23.0.0 milestone May 9, 2025
@systay systay added Type: Bug Component: Query Serving Backport to: release-20.0 Needs to be backport to release-20.0 Backport to: release-21.0 Needs to be backport to release-21.0 Backport to: release-22.0 Needs to be backport to release-22.0 and removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels May 9, 2025
@systay systay changed the title lfix: make merging subqueries great again Fix subquery merging regression introduced in #11379 May 9, 2025
arthurschreiber pushed a commit that referenced this pull request May 9, 2025
arthurschreiber pushed a commit that referenced this pull request May 9, 2025
arthurschreiber pushed a commit that referenced this pull request May 9, 2025
Copy link

codecov bot commented May 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.48%. Comparing base (421aca0) to head (9d27a15).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #18260      +/-   ##
==========================================
+ Coverage   67.47%   67.48%   +0.01%     
==========================================
  Files        1602     1602              
  Lines      262191   262232      +41     
==========================================
+ Hits       176907   176970      +63     
+ Misses      85284    85262      -22     

☔ 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.

arthurschreiber pushed a commit to github/vitess-gh that referenced this pull request May 12, 2025
arthurschreiber added a commit to github/vitess-gh that referenced this pull request May 12, 2025
systay added a commit that referenced this pull request May 13, 2025
systay added a commit that referenced this pull request May 13, 2025
systay added a commit that referenced this pull request May 13, 2025
BenjaminLockhart pushed a commit to HubSpot/vitess that referenced this pull request Jun 9, 2025
timvaillancourt added a commit to slackhq/vitess that referenced this pull request Jun 18, 2025
* [release-22.0] Bump to `v22.0.1-SNAPSHOT` after the `v22.0.0` release (vitessio#18225)

Signed-off-by: Florent Poinsard <[email protected]>
Co-authored-by: Florent Poinsard <[email protected]>

* [release-22.0] fix: Preserve multi-column TupleExpr in tuple simplifier (vitessio#18216) (vitessio#18220)

Signed-off-by: Harshit Gangal <[email protected]>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
Co-authored-by: Harshit Gangal <[email protected]>

* [release-22.0] Properly handle grpc dial errors in the throttler metric aggregation (vitessio#18073) (vitessio#18231)

Signed-off-by: Arthur Schreiber <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
Co-authored-by: Mohamed Hamza <[email protected]>

* [release-22.0] test: TestQueryTimeoutWithShardTargeting fix flaky test (vitessio#18242) (vitessio#18250)

Signed-off-by: Harshit Gangal <[email protected]>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

* [release-22.0] make sure to give MEMBER OF the correct precedence (vitessio#18237) (vitessio#18245)

Signed-off-by: Andres Taylor <[email protected]>
Co-authored-by: Andrés Taylor <[email protected]>

* [release-22.0] Fix evalengine crashes on unexpected types (vitessio#18254) (vitessio#18258)

Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

* [release-22.0] Fix subquery merging regression introduced in vitessio#11379 (vitessio#18260) (vitessio#18263)

Signed-off-by: Arthur Schreiber <[email protected]>
Co-authored-by: Andrés Taylor <[email protected]>

* [release-22.0] json array insert test (vitessio#18284) (vitessio#18286)

Signed-off-by: Harshit Gangal <[email protected]>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
Co-authored-by: Harshit Gangal <[email protected]>

* [release-22.0] Fix `SET` and `START TRANSACTION` in create procedure statements (vitessio#18279) (vitessio#18293)

Signed-off-by: Manan Gupta <[email protected]>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

* [release-22.0] Fix deadlock in semi-sync monitor (vitessio#18276) (vitessio#18290)

Signed-off-by: Manan Gupta <[email protected]>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

* [release-22.0] Upgrade the Golang version to `go1.24.3` (vitessio#18239)

Signed-off-by: GitHub <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Co-authored-by: frouioui <[email protected]>
Co-authored-by: Andres Taylor <[email protected]>

* [release-22.0] Atomic Copy: Handle error that was ignored while streaming tables and log it (vitessio#18313) (vitessio#18316)

Signed-off-by: Rohit Nayak <[email protected]>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

* [release-22.0] fix: handle dml query for None opcode (vitessio#18326) (vitessio#18345)

Signed-off-by: Harshit Gangal <[email protected]>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
Co-authored-by: Harshit Gangal <[email protected]>

* [release-22.0] fix: keep LIMIT/OFFSET even when merging UNION queries (vitessio#18361) (vitessio#18363)

Signed-off-by: Andres Taylor <[email protected]>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
Co-authored-by: Andres Taylor <[email protected]>

* [release-22.0] Fix: Deadlock in `Close` and `write` in semi-sync monitor. (vitessio#18359) (vitessio#18368)

Signed-off-by: Manan Gupta <[email protected]>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

* [release-22.0] Upgrade the Golang version to `go1.24.4` (vitessio#18329)

Signed-off-by: GitHub <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Co-authored-by: frouioui <[email protected]>
Co-authored-by: Manan Gupta <[email protected]>

* [release-22.0] fix version issue when using --mysql-shell-speedup-restore=true (vitessio#18310) (vitessio#18356)

Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

* [release-22.0] Split workflow with flaky vdiff2 e2e test. Skip flaky Migrate test. (vitessio#18300) (vitessio#18334)

Signed-off-by: Rohit Nayak <[email protected]>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

* [release-22.0] Throttler: keep watching topo even on error (vitessio#18223) (vitessio#18322)

Signed-off-by: Shlomi Noach <[email protected]>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>

* [release-22.0] Code Freeze for `v22.0.1` (vitessio#18374)

Signed-off-by: Manan Gupta <[email protected]>

* [release-22.0] Release of `v22.0.1` (vitessio#18375)

Signed-off-by: Manan Gupta <[email protected]>

* add private repo config to new CI file

Signed-off-by: Tim Vaillancourt <[email protected]>

* `make generate_ci_workflows`

Signed-off-by: Tim Vaillancourt <[email protected]>

---------

Signed-off-by: Florent Poinsard <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Arthur Schreiber <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: GitHub <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Tim Vaillancourt <[email protected]>
Co-authored-by: vitess-bot <[email protected]>
Co-authored-by: Florent Poinsard <[email protected]>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
Co-authored-by: Harshit Gangal <[email protected]>
Co-authored-by: Mohamed Hamza <[email protected]>
Co-authored-by: Andrés Taylor <[email protected]>
Co-authored-by: frouioui <[email protected]>
Co-authored-by: Manan Gupta <[email protected]>
Co-authored-by: Manan Gupta <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport to: release-20.0 Needs to be backport to release-20.0 Backport to: release-21.0 Needs to be backport to release-21.0 Backport to: release-22.0 Needs to be backport to release-22.0 Component: Query Serving Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug Report: Subqueries that join on vindex columns don't get merged correctly
3 participants