Skip to content
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

sql: replace all map[sqlbase.ColumnID]int types with util.FastIntMap #37801

Open
nvanbenschoten opened this issue May 24, 2019 · 4 comments

Comments

@nvanbenschoten
Copy link
Member

@nvanbenschoten nvanbenschoten commented May 24, 2019

pkg/sql is littered with mappings from column IDs to projection/row/other indexes. The package uses the type map[sqlbase.ColumnID]int to represent these mappings.

In almost all cases, we would be better off using util.FastIntMap, which can avoid a map allocation if the column ID never goes above 31 and the index never goes above 14. Any table that wouldn't be considered "wide" meets these requirements.

We should replace all instances of map[sqlbase.ColumnID]int with a util.FastIntMap in pkg/sql. It looks like pkg/sql/opt already does this and aliases util.FastIntMap to ColMap. We might consider sharing the same alias across these packages.

cc. @jordanlewis @RaduBerinde is there any reason that I'm missing not to do this?

@jordanlewis

This comment has been minimized.

Copy link
Member

@jordanlewis jordanlewis commented Jun 5, 2019

No, this generally seems like a pretty good idea to me.

@RaduBerinde

This comment has been minimized.

Copy link
Member

@RaduBerinde RaduBerinde commented Jun 5, 2019

In opt we would really like to have a version that uses ColumnID instead of int (casting is annoying and leads to occasional bugs which would have been caught by the type system). Last time we tried to write a wrapper, it was slower (some wrappers were not inlined); maybe this is better in more recent versions of Go.

@jordanlewis

This comment has been minimized.

Copy link
Member

@jordanlewis jordanlewis commented Jun 6, 2019

@RaduBerinde what do you mean by wrapper exactly? A thing that takes ColumnID and turns it into int to pass to the actual implementation?

We could use execgen to generate a couple of copies of FastIntMap, though this is pretty lame considering ColumnID is just an int under the hood...

@RaduBerinde

This comment has been minimized.

Copy link
Member

@RaduBerinde RaduBerinde commented Jun 6, 2019

Yeah, exactly. The wrapper would be another type and would have to reimplement all methods (passing them through with conversions). I was thinking about ColSet more than ColMap but I think it applies to both.

Yes, generating multiple versions is a possibility (though an annoying one).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.