Skip to content

opt: do not produce unneeded PK cols in inverted index scans #147490

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

Merged
merged 1 commit into from
Jun 1, 2025

Conversation

mgartner
Copy link
Collaborator

A minor bug has been fixed where an inverted index scan could produce
more PK columns than necessary. This caused test-only assertion failures
but does not cause any known, user-visible bugs. The bug was caused by
the incorrect assumption that all inverted index scans need to produces
all PK columns. In actuality, an inverted index scan can produce a
subset of PK columns if the original scan produces a subset of PK
columns and neither an inverted filter nor an index join are needed.

Fixes #143070

Release note: None

@mgartner mgartner requested a review from a team May 29, 2025 13:51
@mgartner mgartner requested a review from a team as a code owner May 29, 2025 13:51
@mgartner mgartner requested review from michae2 and removed request for a team May 29, 2025 13:51
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner mgartner requested a review from DrewKimball May 29, 2025 21:27
Copy link
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @michae2)


pkg/sql/opt/xform/select_funcs.go line 974 at r1 (raw file):

		// the original scan.
		newScanPrivate.Cols = pkCols.Copy()
		newScanPrivate.Cols.IntersectionWith(scanPrivate.Cols)

[super nit] you can make this a one-liner :)

newScanPrivate.Cols = pkCols.Intersection(scanPrivate.Cols)

pkg/sql/opt/xform/select_funcs.go line 1009 at r1 (raw file):

		// by the scan.
		filters = sb.AddSelectAfterSplit(filters, pkCols)
		if needIndexJoin {

If we don't need an index join but do need an inverted filter, do we need to project away unneeded primary key columns?

A minor bug has been fixed where an inverted index scan could produce
more PK columns than necessary. This caused test-only assertion failures
but does not cause any known, user-visible bugs. The bug was caused by
the incorrect assumption that all inverted index scans need to produces
all PK columns. In actuality, an inverted index scan can produce a
subset of PK columns if the original scan produces a subset of PK
columns and neither an inverted filter nor an index join are needed.

Fixes cockroachdb#143070

Release note: None
@mgartner mgartner force-pushed the 143070-inverted-scan-cols branch from 4a291ff to 2e0b93c Compare May 31, 2025 15:56
Copy link
Collaborator Author

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @michae2)


pkg/sql/opt/xform/select_funcs.go line 974 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

[super nit] you can make this a one-liner :)

newScanPrivate.Cols = pkCols.Intersection(scanPrivate.Cols)

Ya, I debated it. I wanted to make the copy explicit since it's critical. I switched to your suggestion and added a note.


pkg/sql/opt/xform/select_funcs.go line 1009 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

If we don't need an index join but do need an inverted filter, do we need to project away unneeded primary key columns?

Yes, that's already handled by the scan builder. See #141670 and the test directly about the one I added in this PR.

@mgartner
Copy link
Collaborator Author

Yes, that's already handled by the scan builder. See #141670 and the test directly about the one I added in this PR.

s/about/above

Copy link
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

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

:lgtm: Nice work!

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @michae2)

@mgartner mgartner added the backport-all Flags PRs that need to be backported to all supported release branches label Jun 1, 2025
@mgartner
Copy link
Collaborator Author

mgartner commented Jun 1, 2025

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 1, 2025

@craig craig bot merged commit 8fed6e8 into cockroachdb:master Jun 1, 2025
31 of 32 checks passed
Copy link

blathers-crl bot commented Jun 1, 2025

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details.

error creating merge commit from 2e0b93c to blathers/backport-release-23.2-147490: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch release-23.2 failed. See errors above.


💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details.

error creating merge commit from 2e0b93c to blathers/backport-release-24.1-147490: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch release-24.1 failed. See errors above.


💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details.

error creating merge commit from 2e0b93c to blathers/backport-release-24.3-147490: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch release-24.3 failed. See errors above.


💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details.

error creating merge commit from 2e0b93c to blathers/backport-release-25.1-147490: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch release-25.1 failed. See errors above.


💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@mgartner mgartner deleted the 143070-inverted-scan-cols branch June 2, 2025 11:42
@mgartner mgartner added backport-25.2.x Flags PRs that need to be backported to 25.2 and removed backport-all Flags PRs that need to be backported to all supported release branches labels Jun 2, 2025
@mgartner
Copy link
Collaborator Author

mgartner commented Jun 2, 2025

I'm going to hold-off on backporting to anything other than 25.2. If we continue to see these failures on older release branches, we can try to backport this, but we'll likely also need to backport #141670.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-25.2.x Flags PRs that need to be backported to 25.2 backport-failed target-release-25.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

roachtest: output cols mismatch with inverted index scans
3 participants