sql: add Index interface and TableDescriptor methods in catalog #58678
Conversation
|
This change is |
|
This is like #58273 but with less race conditions! |
postamar
force-pushed
the
use-new-index-interface
branch
from
6c3da45
to
3af89a9
Compare
|
I took the opportunity to clean up the commits a bit as well, I found the diffs were becoming a bit ugly, especially since this all warrants another look (evidently). As a result, the meat&potatoes of this change is all in the first commit. I used |
There was a problem hiding this comment.
I only looked at the first commit. Doesn't this still initialize the cache lazily? I was hoping we wouldn't even need any sort of synchronization on the cache if we just force all tabledesc.Immutable initialization to go through some constructor, and initialize the cache in that function. Maybe I'm missing something.
Reviewable status:
complete! 0 of 0 LGTMs obtained
|
No, you're right, it's still lazy, just less so. I guess I got caught up in with this idea that designing structs so that their zero values are meaningful is idiomatic go. I just looked and I don't think there's anything wrong with building the cache eagerly in |
postamar
force-pushed
the
use-new-index-interface
branch
from
3af89a9
to
1b3bfc6
Compare
|
To the surprise of no one, getting rid of the laziness makes for better code. What was I thinking. |
postamar
force-pushed
the
use-new-index-interface
branch
from
1b3bfc6
to
76af804
Compare
|
RFAL @lucy-zhang @ajwerner |
postamar
requested review from
miretskiy and
a team
and removed request for
a team
postamar
force-pushed
the
use-new-index-interface
branch
from
76af804
to
c9ff4a5
Compare
|
Rebased on master to fix conflicts. |
There was a problem hiding this comment.
I'm assuming the later commits didn't change. 
Reviewed 10 of 16 files at r1, 6 of 7 files at r2, 1 of 1 files at r3.
Reviewable status:
|
Thanks. In addition to the cursory visual checks I'd already done, I just checked to see how much the later commits changed with the following method, for each later commit
I didn't find any surprises. It's not perfect but I figure git is better at diffing things at scale than humans so this leaves me reasonably confident that the later commits are as they should be. |
There was a problem hiding this comment.
Reviewed 7 of 16 files at r1, 5 of 7 files at r2.
Reviewable status:
Previously, the catalog.TableDescriptor interface and its implementing types would liberally return descpb.IndexDescriptor values, pointers and slices. In an effort to stop manipulating such protos directly, this patch introduces a catalog.Index interface to encapsulate it. In order to enventually propagate this change throughout the code base, this patch marks some existing catalog.TableDescriptor methods as deprecated and introduces new ones to eventually replace them. Partially addresses cockroachdb#57465. Release note: None
Previously, these methods would be called on a table descriptor interface (or backing struct) to obtain a slice of descpb.IndexDescriptors. This patch removes these calls, along with the method definitions, in favour of new methods which use the catalog.Index interface type instead. Partially addresses cockroachdb#57465. Release note: None
Previously, this method would be called on a table descriptor interface (or backing struct) to obtain a slice of descpb.IndexDescriptor pointers. This patch removes these calls, along with the method definitions, in favour of new methods which use the catalog.Index interface type instead. Partially addresses cockroachdb#57465. Release note: None
Previously, this method would be called on a table descriptor interface (or backing struct) to apply a function on the *descpb.IndexDescriptor of each of the table's non-drop indexes. This patch removes these calls, along with the method definition, in favour of new methods which use the catalog.Index interface type instead. Partially addresses cockroachdb#57465. Release note: None
Previously, this method would be called on a table descriptor interface (or backing struct) to apply a function on the *descpb.IndexDescriptor of each of the table's indexes. This patch removes these calls, along with the method definition, in favour of new methods which use the catalog.Index interface type instead. Partially addresses cockroachdb#57465. Release note: None
Previously, this method would be called on a table descriptor interface (or backing struct) to return a target *descpb.IndexDescriptor. This patch removes these calls, along with the method definition, in favour of new methods which use the catalog.Index interface type instead. Partially addresses cockroachdb#57465. Release note: None
Previously, this method would be called on a table descriptor interface (or backing struct) to return a target *descpb.IndexDescriptor. This patch removes these calls, along with the method definition, in favour of new methods which use the catalog.Index interface type instead. Partially addresses cockroachdb#57465. Release note: None
Previously, this method would be called on a table descriptor interface (or backing struct) to obtain a slice of descpb.IndexDescriptors. This patch removes these calls, along with the method definition, in favour of new methods which use the catalog.Index interface type instead. Partially addresses cockroachdb#57465. Release note: None
Previously, this method would be called on a table descriptor interface (or backing struct) to obtain the primary index descriptor in the form of a *descpb.IndexDescriptor. This patch removes these calls, along with the method definition, in favour of new methods which use the catalog.Index interface type instead. Partially addresses cockroachdb#57465. Release note: None
Previously, the name PrimaryIndexInterface had been chosen because a deprecated GetPrimaryIndex() method already existed and because PrimaryIndex is an existing field in descpb.IndexDescriptor which is embedded in the structs implementing the catalog.TableDescriptor interface. This patch renames the method to GetPrimaryIndex: although its return type differs it has the same meaning as the recently-removed method of the same name. Release note: None
postamar
force-pushed
the
use-new-index-interface
branch
from
c9ff4a5
to
3e2a4e7
Compare
|
Thanks for the reviews everyone. I had to force-push again to address another conflict. |
|
bors r+ |
|
Build succeeded: |
59789: sql: add catalog.Column interface, replace catalog.TableDescriptor methods for columns r=postamar a=postamar In an effort to not directly use `descpb.ColumnDescriptor` protos everywhere, this multi-commit PR: 1. introduces a new `catalog.Column` interface to encapsulate them, and a bunch of new methods in `catalog.TableDescriptor` to replace those which have `descpb.ColumnDescriptor` in their signatures, this is done in the first commit; 2. replaces calls to the old methods with calls to the new methods throughout the codebase in the subsequent commits. This breakdown into multiple commits is done to ease review, most of the substantial changes are in the first commit, the others follow through with the change and so are quite noisy. Fixes #59805. See also the parent issue #56306, as well as the related issue #57465 which tracks the same work I've already done for `descpb.IndexDescriptor`. In this PR I reused many of the same patterns I introduced in the corresponding PR #58678. Subsequent work should consist in propagating this `descpb.ColumnDescriptor -> catalog.Column` change further throughout the codebase. Co-authored-by: Marius Posta <marius@cockroachlabs.com>
Previously, the catalog.TableDescriptor interface and its implementing
types would liberally return descpb.IndexDescriptor values, pointers and
slices. In an effort to stop manipulating such protos directly, this
patch introduces a catalog.Index interface to encapsulate it.
Addresses #57465.
Release note: None