semantics: Use a BitSet #11819

vmg · 2022-11-25T14:57:24Z

Following up on #11817 -- the bug was caused because our current implementation of TableSet is not strictly comparable with equality (i.e. ==).

This PR fixes the issue in depth by bringing in a new implementation of an immutable bitset that is safe to compare with equality and to be used as a key in a hashmap (something we also do quite often).

Fixes #11805

cc @systay

Signed-off-by: Andres Taylor <andres@planetscale.com>

vitess-bot · 2022-11-25T14:57:26Z

Review Checklist

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

General

Ensure that the Pull Request has a descriptive title.
If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.

If a new flag is being introduced:

Is it really necessary to add this flag?
Flag names should be clear and intuitive (as far as possible)
Help text should be descriptive.
Flag names should use dashes (-) as word separators rather than underscores (_).

If a workflow is added or modified:

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

Bug fixes

There should be at least one unit or end-to-end test.
The Pull Request description should include a link to an issue that describes the bug.

Non-trivial changes

There should be some code comments as to why things are implemented the way they are.

New/Existing features

Should be documented, either by modifying the existing documentation or creating new documentation.
New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

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

Signed-off-by: Vicent Marti <vmg@strn.cat>

vmg · 2022-11-25T15:02:19Z

There's a new commit that fixes some of the superluous renaming but it's not showing up in the web UI. I think GitHub is having issues again.

Signed-off-by: Vicent Marti <vmg@strn.cat>

Signed-off-by: Andres Taylor <andres@planetscale.com>

harshit-gangal · 2022-11-27T17:01:08Z

@vmg could you add more documentation to the bitset implementation about how it works? Also, any place where I can read more about it?

arthurschreiber · 2022-11-27T17:26:38Z

Sorry for the nitpicking, but I think the filename is still incorrect - it's currently tablet_set.go instead of table_set.go. 🙈

harshit-gangal · 2022-11-28T05:18:44Z

go/vt/vtgate/semantics/bitset/bitset.go

+ if len(words) == 0 {
+ return ""
+ }
+ return *(*Bitset)(unsafe.Pointer(&words))


Does this get the memory address from go runtime?

harshit-gangal · 2022-11-28T07:20:05Z

go/vt/vtgate/semantics/bitset/bitset.go

+const singleton = "\x00\x00\x00\x01\x00\x00\x00\x02\x00\x00\x00\x04\x00\x00\x00\x08\x00\x00\x00\x10\x00\x00\x00\x20\x00\x00\x00\x40\x00\x00\x00\x80"
+
+// Single returns a new Bitset where only the given bit is set.
+// If the given bit is less than 32, Single does not allocate to create a new Bitset.
+func Single(bit int) Bitset {


what is the limit to the number of tableSet that can exist now?

There is no limit! Any bitset larger than 32 will allocate, but you can make it as large as you want.

harshit-gangal · 2022-11-28T08:17:08Z

go/vt/vtgate/semantics/tablet_set.go

+// TableSetFromIds returns TableSet for all the id passed in argument.
+func TableSetFromIds(tids ...int) (ts TableSet) {
+ return TableSet(bitset.Build(tids...))
+}


this method is not used in the codebase which will also imply that Build method is not required in bitset

We intend to use this method in some PlanetScale-internal projects!

I would suggest having at least a test for it so that if anyone uses/modifies it here, does not break the usage.

harshit-gangal · 2022-11-28T08:20:49Z

@vmg could you add more documentation to the bitset implementation about how it works? Also, any place where I can read more about it?

it was nice reading some math-intensive code.

harshit-gangal · 2022-11-28T08:23:55Z

go/vt/vtgate/semantics/bitset/bitset.go

+func (bs Bitset) ForEach(yield func(int)) {
+ for i := 0; i < len(bs); i++ {
+ bitset := bs[i]
+ for bitset != 0 {
+ t := bitset & -bitset
+ r := bits.TrailingZeros8(bitset)
+ yield(i*bitsetWidth + r)
+ bitset ^= t
+ }
+ }
+}
+


an example of how this works will help. A general example for overall implementation will also work.

Signed-off-by: Vicent Marti <vmg@strn.cat>

vmg · 2022-11-28T09:37:09Z

Added comments on the tricky parts of the implementation 👌

vitess-bot · 2022-11-28T10:38:12Z

I was unable to backport this Pull Request to the following branches: release-15.0.

* make sure planner works well with more than 64 tables Signed-off-by: Andres Taylor <andres@planetscale.com> * semantics: use an immutable bitset Signed-off-by: Vicent Marti <vmg@strn.cat> * bitset: add documentation Signed-off-by: Vicent Marti <vmg@strn.cat> * bitset: add license Signed-off-by: Vicent Marti <vmg@strn.cat> * use IsEmpty and NonEmpty more Signed-off-by: Andres Taylor <andres@planetscale.com> * rename file Signed-off-by: Andres Taylor <andres@planetscale.com> * bitset: better documentation Signed-off-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Vicent Marti <vmg@strn.cat> Co-authored-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Manan Gupta <manan@planetscale.com>

* make sure planner works well with more than 64 tables Signed-off-by: Andres Taylor <andres@planetscale.com> * semantics: use an immutable bitset Signed-off-by: Vicent Marti <vmg@strn.cat> * bitset: add documentation Signed-off-by: Vicent Marti <vmg@strn.cat> * bitset: add license Signed-off-by: Vicent Marti <vmg@strn.cat> * use IsEmpty and NonEmpty more Signed-off-by: Andres Taylor <andres@planetscale.com> * rename file Signed-off-by: Andres Taylor <andres@planetscale.com> * bitset: better documentation Signed-off-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Vicent Marti <vmg@strn.cat> Co-authored-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Manan Gupta <manan@planetscale.com> Co-authored-by: Vicent Martí <42793+vmg@users.noreply.github.com> Co-authored-by: Andres Taylor <andres@planetscale.com>

* semantics: Use a BitSet (#11819) (#11837) * make sure planner works well with more than 64 tables Signed-off-by: Andres Taylor <andres@planetscale.com> * semantics: use an immutable bitset Signed-off-by: Vicent Marti <vmg@strn.cat> * bitset: add documentation Signed-off-by: Vicent Marti <vmg@strn.cat> * bitset: add license Signed-off-by: Vicent Marti <vmg@strn.cat> * use IsEmpty and NonEmpty more Signed-off-by: Andres Taylor <andres@planetscale.com> * rename file Signed-off-by: Andres Taylor <andres@planetscale.com> * bitset: better documentation Signed-off-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Vicent Marti <vmg@strn.cat> Co-authored-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Manan Gupta <manan@planetscale.com> Co-authored-by: Vicent Martí <42793+vmg@users.noreply.github.com> Co-authored-by: Andres Taylor <andres@planetscale.com> * boostplan: use the new TableSet API Signed-off-by: Vicent Marti <vmg@strn.cat> * [planner] Better AST equality (#11867) (#1387) * [planner] Better AST equality (#11867) * make it possible to do deep comparisons with custom logic Signed-off-by: Andres Taylor <andres@planetscale.com> * use the new EqualS function to do deep semantic equality Signed-off-by: Andres Taylor <andres@planetscale.com> * re gen the asthelpergen integration test data Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * update equal gen comment Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * test: add unit tests showing the new EqualsExpr functionality Signed-off-by: Andres Taylor <andres@planetscale.com> * [planner] use the semantic equality in more places Signed-off-by: Andres Taylor <andres@planetscale.com> * comments - update to reflect new code Signed-off-by: Andres Taylor <andres@planetscale.com> * small fixes around tests Signed-off-by: Andres Taylor <andres@planetscale.com> * update integration test build Signed-off-by: Andres Taylor <andres@planetscale.com> * use semantic equality for Anding together expressions Signed-off-by: Andres Taylor <andres@planetscale.com> * test: simplify test Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> Co-authored-by: Florent Poinsard <florent.poinsard@outlook.fr> * boostplan: use new AST equality helpers Signed-off-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> Signed-off-by: Vicent Marti <vmg@strn.cat> Co-authored-by: Florent Poinsard <florent.poinsard@outlook.fr> Co-authored-by: Vicent Marti <vmg@strn.cat> * Fix deprecated ioutil usage in custom backups (#1388) These are our internal implementations for backup so they won't get updated upstream to fix this deprecation, so we have to do it here. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Signed-off-by: Vicent Marti <vmg@strn.cat> * Experiment boost queries on Gen4 (#1373) * watcher: add support for experiments Signed-off-by: Vicent Marti <vmg@strn.cat> * more logic around comparing results Co-authored-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * add engine primitive that can compare boost and gen4 results Signed-off-by: Andres Taylor <andres@planetscale.com> * link the gen4 vs boost comparison to the plan time in vtgate Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * refactor the utils comparison methods and add fail on error to boost experiments Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * add an enum to experiments Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * rename experiments to querycomparison Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * add topo rpc endpoint in vtboost and vtctld Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * add vtctldclient command for setquerycomparisons Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * change fakeControllerClient to implement DRPCControllerServiceClient Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * self-review Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * run TryExecute async when failure mode is set to LOG Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * run TryStreamExecute async when failure mode is set to LOG Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * Introduce a pflag file for boost Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * fix proto files Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * fix executor error Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * re gen proto Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * apply review suggestions Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * Rename query comparison to Science Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * Filter SetScience per cluster Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * Simplify the set science in tests and optimize the use of vcursor in BoostCompare Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * addition of context to BoostCompare Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * remove BackgroundVCursor Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * topo: simplify Signed-off-by: Vicent Marti <vmg@strn.cat> * use ctx.Background Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * gofmt Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * make proto Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * endtoend: fix test output Signed-off-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> Signed-off-by: Andres Taylor <andres@planetscale.com> Co-authored-by: Vicent Marti <vmg@strn.cat> Co-authored-by: Andres Taylor <andres@planetscale.com> * boost: Go 1.19 cleanups (#1403) * common: use Go 1.19 atomics Signed-off-by: Vicent Marti <vmg@strn.cat> * Fix go.mod Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Signed-off-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Co-authored-by: Dirkjan Bussink <d.bussink@gmail.com> * add group by and original expressions (#1423) * add group by and original expressions Signed-off-by: Andres Taylor <andres@planetscale.com> * incorporator: allow disabling upquery generation Signed-off-by: Vicent Marti <vmg@strn.cat> * integration: remove unused test Signed-off-by: Vicent Marti <vmg@strn.cat> * don't rewrite colnames in the original query while planning upqueries Signed-off-by: Andres Taylor <andres@planetscale.com> * boostplan: add missing upquery rewrite Signed-off-by: Vicent Marti <vmg@strn.cat> * incorporator: revert optional upqueries Signed-off-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Vicent Marti <vmg@strn.cat> Co-authored-by: Vicent Marti <vmg@strn.cat> * [planner] Schema information on the information_schema views (#11941) * add info_schema information Signed-off-by: Andres Taylor <andres@planetscale.com> * add SchemaInformation handling for info_schema tables Signed-off-by: Andres Taylor <andres@planetscale.com> * fix bad test query Signed-off-by: Andres Taylor <andres@planetscale.com> * add support for information_schema on mysql 5.7 Signed-off-by: Andres Taylor <andres@planetscale.com> * columns sorted just like mysql, and tests for 5.7 Signed-off-by: Andres Taylor <andres@planetscale.com> * test: skip test that should not run Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Co-authored-by: Manan Gupta <35839558+GuptaManan100@users.noreply.github.com> Co-authored-by: Andres Taylor <andres@planetscale.com> Co-authored-by: Florent Poinsard <florent.poinsard@outlook.fr> Co-authored-by: Dirkjan Bussink <d.bussink@gmail.com> Co-authored-by: FlorentP <35779988+frouioui@users.noreply.github.com>

make sure planner works well with more than 64 tables

9490976

Signed-off-by: Andres Taylor <andres@planetscale.com>

vmg requested review from harshit-gangal, systay, frouioui, GuptaManan100, mattlord and deepthi as code owners November 25, 2022 14:57

semantics: use an immutable bitset

ed9c036

Signed-off-by: Vicent Marti <vmg@strn.cat>

vmg force-pushed the vmg/bitset branch from 80d9020 to ed9c036 Compare November 25, 2022 15:02

vmg and others added 3 commits November 25, 2022 16:10

bitset: add documentation

f6ea980

Signed-off-by: Vicent Marti <vmg@strn.cat>

bitset: add license

e8692b3

Signed-off-by: Vicent Marti <vmg@strn.cat>

use IsEmpty and NonEmpty more

c97285b

Signed-off-by: Andres Taylor <andres@planetscale.com>

systay approved these changes Nov 25, 2022

View reviewed changes

systay added Type: Bug Component: Query Serving Backport to: release-15.0 labels Nov 25, 2022

This comment was marked as resolved.

Sign in to view

rename file

5030fcf

Signed-off-by: Andres Taylor <andres@planetscale.com>

harshit-gangal reviewed Nov 28, 2022

View reviewed changes

frouioui approved these changes Nov 28, 2022

View reviewed changes

bitset: better documentation

9671910

Signed-off-by: Vicent Marti <vmg@strn.cat>

harshit-gangal approved these changes Nov 28, 2022

View reviewed changes

GuptaManan100 approved these changes Nov 28, 2022

View reviewed changes

vmg merged commit 9a9bd43 into vitessio:main Nov 28, 2022

vmg deleted the vmg/bitset branch November 28, 2022 10:37

arthurschreiber mentioned this pull request Nov 28, 2022

Planner Bug: bug: not supported pushWeightStringForDistinct on *planbuilder.limit #11797

Closed

3 tasks

GuptaManan100 mentioned this pull request Nov 29, 2022

[release-15.0] semantics: Use a BitSet (#11819) #11837

Merged

austenLacy mentioned this pull request Apr 26, 2023

Fixes the SwitchTraffic bug that wasn't respecting --dry_run for readonly and replica tablets during a resharding event Shopify/vitess#91

Merged

austenLacy mentioned this pull request Aug 1, 2023

Custom jwt http ACL policy Shopify/vitess#114

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

semantics: Use a BitSet #11819

semantics: Use a BitSet #11819

vmg commented Nov 25, 2022 •

edited by systay

Loading

vitess-bot bot commented Nov 25, 2022 •

edited by systay

Loading

vmg commented Nov 25, 2022

This comment was marked as resolved.

harshit-gangal commented Nov 27, 2022

arthurschreiber commented Nov 27, 2022

harshit-gangal Nov 28, 2022

harshit-gangal Nov 28, 2022

vmg Nov 28, 2022

harshit-gangal Nov 28, 2022

vmg Nov 28, 2022

harshit-gangal Nov 28, 2022 •

edited

Loading

harshit-gangal commented Nov 28, 2022

harshit-gangal Nov 28, 2022

vmg commented Nov 28, 2022

vitess-bot bot commented Nov 28, 2022

semantics: Use a BitSet #11819

semantics: Use a BitSet #11819

Conversation

vmg commented Nov 25, 2022 • edited by systay Loading

vitess-bot bot commented Nov 25, 2022 • edited by systay Loading

Review Checklist

General

If a new flag is being introduced:

If a workflow is added or modified:

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

vmg commented Nov 25, 2022

This comment was marked as resolved.

harshit-gangal commented Nov 27, 2022

arthurschreiber commented Nov 27, 2022

harshit-gangal Nov 28, 2022

Choose a reason for hiding this comment

harshit-gangal Nov 28, 2022

Choose a reason for hiding this comment

vmg Nov 28, 2022

Choose a reason for hiding this comment

harshit-gangal Nov 28, 2022

Choose a reason for hiding this comment

vmg Nov 28, 2022

Choose a reason for hiding this comment

harshit-gangal Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

harshit-gangal commented Nov 28, 2022

harshit-gangal Nov 28, 2022

Choose a reason for hiding this comment

vmg commented Nov 28, 2022

vitess-bot bot commented Nov 28, 2022

vmg commented Nov 25, 2022 •

edited by systay

Loading

vitess-bot bot commented Nov 25, 2022 •

edited by systay

Loading

harshit-gangal Nov 28, 2022 •

edited

Loading