Use faster set for column IDs in schemaexpr by mgartner · Pull Request #49819 · cockroachdb/cockroach

mgartner · 2020-06-03T02:01:55Z

sqlbase: add ColSet wrapper for util.FastIntSet of ColumnID

There are numerous places where a map[sqlbase.ColumnID]struct{} or a
util.FastIntSet is used to represent a set of sqlbase.ColumnID. This
commit adds a typed wrapper around util.FastIntSet which is an
efficient and ergonimic replacement for maps and util.FastIntSet.

Release note: None

schemaexpr: use sqlbase.ColSet instead of maps

This commit replaces maps used as sets of integers with sqlbase.ColSet
because it is a more efficient set implementation.

Release note: None

cockroach-teamcity · 2020-06-03T02:02:02Z

This change is

RaduBerinde

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @mjibson, @RaduBerinde, and @rytaft)

pkg/sql/schemaexpr/column.go, line 154 at r1 (raw file):

func replaceVars(
	desc *sqlbase.MutableTableDescriptor, rootExpr tree.Expr,
) (tree.Expr, util.FastIntSet, error) {

I'd name this return arg so it's more evident what it is, e.g. _ tree.Expr, columnIDs util.FastIntSet, _ error

pkg/sql/schemaexpr/computed_column.go, line 61 at r1 (raw file):

	}

	var dependencies util.FastIntSet

[nit] Unfortunately it's not easy to tell what the numbers in the set are, maybe depColumnIDs. Or, if there is or we expect wider-spread usage of sets of sqlbase.ColumnID, we may want to make a nicely typed wrapper like opt.ColSet.

mgartner

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mjibson, @RaduBerinde, and @rytaft)

pkg/sql/schemaexpr/column.go, line 154 at r1 (raw file):

Previously, RaduBerinde wrote…

I'd name this return arg so it's more evident what it is, e.g. _ tree.Expr, columnIDs util.FastIntSet, _ error

Now that I've added a typed wrapper, I'm thinking this is OK to be unnamed. Do you agree?

pkg/sql/schemaexpr/computed_column.go, line 61 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] Unfortunately it's not easy to tell what the numbers in the set are, maybe depColumnIDs. Or, if there is or we expect wider-spread usage of sets of sqlbase.ColumnID, we may want to make a nicely typed wrapper like opt.ColSet.

Good point. I've updated the name, and I've also created the sqlbase.ColSet typed wrapper, since it looks like there's quite a few places that could use it.

Is that name good, or would sqlbase.ColIdSet or sqlbase.ColumnIDSet be better?

My quick searching of some places this could be used:

RaduBerinde · 2020-06-03T15:56:48Z

pkg/sql/schemaexpr/computed_column.go, line 61 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Good point. I've updated the name, and I've also created the sqlbase.ColSet typed wrapper, since it looks like there's quite a few places that could use it.

Is that name good, or would sqlbase.ColIdSet or sqlbase.ColumnIDSet be better?

My quick searching of some places this could be used:

Nice! Maybe TableColSet? because you can only reasonably use it for columns of a single table (ids are unique only within each table)

There are numerous places where a `map[sqlbase.ColumnID]struct{}` or a `util.FastIntSet` is used to represent a set of `sqlbase.ColumnID`. This commit adds a typed wrapper around `util.FastIntSet` which is an efficient and ergonomic replacement for maps and `util.FastIntSet`. Release note: None

pkg/sql/schemaexpr/column.go

rohany · 2020-06-03T19:20:01Z

pkg/sql/sqlbase/table_col_set.go

+import "github.com/cockroachdb/cockroach/pkg/util"
+
+// TableColSet efficiently stores an unordered set of column ids.
+type TableColSet struct {


What is the reason for wrapping util.FastIntSet here? As far as I can tell all the methods just call into util.FastIntSet.

It's so it's strongly typed, which makes it evident what it stores and you don't have to cast ColumnIDs to ints and back.

I guess, but it also feels like a good amount of duplication.

After working without a wrapper for opt.ColSet for a few releases (until Go implemented mid-stack inlining) I can tell you that the duplication is worth it.

Sounds good -- i haven't had to use this extensively to really have an opinion.

I think the bit of duplication is worth it for the clarity and conciseness it provides when using TableColSet.

rytaft

Reviewed 5 of 5 files at r2, 5 of 6 files at r3, 2 of 2 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner, @mjibson, @RaduBerinde, and @rohany)

This commit replaces maps used as sets of integers with sqlbase.TableColSet because it is a more efficient set implementation. Release note: None

mgartner · 2020-06-04T18:23:45Z

bors r+

craig · 2020-06-04T18:26:24Z

Merge conflict (retrying...)

craig · 2020-06-04T19:11:37Z

Build succeeded

GitHub CI (Cockroach)

mgartner requested review from RaduBerinde, madelynnblue and rytaft June 3, 2020 02:01

RaduBerinde approved these changes Jun 3, 2020

View reviewed changes

mgartner force-pushed the use-fast-int-set-in-schemaexpr branch 3 times, most recently from 65ccc0a to ae9c179 Compare June 3, 2020 15:16

mgartner commented Jun 3, 2020

View reviewed changes

mgartner changed the title ~~sql: use util.FastIntSet in schemaexpr~~ Use faster set for column IDs in schemaexpr Jun 3, 2020

mgartner force-pushed the use-fast-int-set-in-schemaexpr branch from ae9c179 to c63ded6 Compare June 3, 2020 15:29

mgartner force-pushed the use-fast-int-set-in-schemaexpr branch from c63ded6 to 166d492 Compare June 3, 2020 17:49

mgartner mentioned this pull request Jun 3, 2020

sql: serialize UDTs in expressions in a stable way #49565

Merged

mgartner force-pushed the use-fast-int-set-in-schemaexpr branch from 166d492 to 3896a63 Compare June 3, 2020 18:29

rohany reviewed Jun 3, 2020

View reviewed changes

mgartner force-pushed the use-fast-int-set-in-schemaexpr branch 2 times, most recently from b44a672 to 0a54fd8 Compare June 3, 2020 22:38

rytaft approved these changes Jun 3, 2020

View reviewed changes

schemaexpr: use sqlbase.TableColSet instead of maps

f61f13e

This commit replaces maps used as sets of integers with sqlbase.TableColSet because it is a more efficient set implementation. Release note: None

mgartner force-pushed the use-fast-int-set-in-schemaexpr branch from 0a54fd8 to f61f13e Compare June 4, 2020 03:15

craig bot merged commit 994d306 into cockroachdb:master Jun 4, 2020

mgartner deleted the use-fast-int-set-in-schemaexpr branch June 4, 2020 19:44

Conversation

mgartner commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

sqlbase: add ColSet wrapper for util.FastIntSet of ColumnID

schemaexpr: use sqlbase.ColSet instead of maps

Uh oh!

cockroach-teamcity commented Jun 3, 2020

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

mgartner left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde commented Jun 3, 2020

Uh oh!

Uh oh!

rohany Jun 3, 2020

Choose a reason for hiding this comment

Uh oh!

RaduBerinde Jun 3, 2020

Choose a reason for hiding this comment

Uh oh!

rohany Jun 3, 2020

Choose a reason for hiding this comment

Uh oh!

RaduBerinde Jun 3, 2020

Choose a reason for hiding this comment

Uh oh!

rohany Jun 3, 2020

Choose a reason for hiding this comment

Uh oh!

mgartner Jun 3, 2020

Choose a reason for hiding this comment

Uh oh!

rytaft left a comment

Choose a reason for hiding this comment

Uh oh!

mgartner commented Jun 4, 2020

Uh oh!

craig bot commented Jun 4, 2020

Merge conflict (retrying...)

Uh oh!

craig bot commented Jun 4, 2020

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mgartner commented Jun 3, 2020 •

edited

Loading