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

tree: remove roachpb references #80684

Merged
merged 3 commits into from
May 2, 2022
Merged

Conversation

otan
Copy link
Contributor

@otan otan commented Apr 28, 2022

See individual commits for details.

@otan otan requested a review from ajwerner April 28, 2022 04:59
@otan otan requested review from a team as code owners April 28, 2022 04:59
@otan otan requested a review from a team April 28, 2022 04:59
@otan otan requested a review from a team as a code owner April 28, 2022 04:59
@otan otan requested review from msbutler and removed request for a team April 28, 2022 04:59
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, 2 of 2 files at r2, 7 of 7 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @msbutler, and @otan)


pkg/sql/sem/tree/tenant.go line 29 at r1 (raw file):

	// TODO(knz): None of this complexity would be needed if the tenant
	// ID was specified using an Expr here. This can be removed once the
	// backup target syntax accepts expressions for tenant targets.

this comment is pretty stale now given we really actively rely on Specified and it's not "extra complexity"

Code quote:

	// Specified is set to true when the TENANT clause was specified in
	// the backup target input syntax. We need this, instead of relying
	// on TenantID.IsSet(), because we need a special marker for
	// the case when the value was anonymized. In other places, the
	// value used for anonymized integer literals is 0, but we can't use
	// 0 for TenantID as this is refused during parsing, nor can we use
	// any other value since all non-zero integers are valid tenant IDs.
	//
	// TODO(knz): None of this complexity would be needed if the tenant
	// ID was specified using an Expr here. This can be removed once the
	// backup target syntax accepts expressions for tenant targets.

pkg/sql/sem/tree/tenant.go line 37 at r1 (raw file):

// IsSet returns whether the TenantID is set.
func (t *TenantID) IsSet() bool {
	return t.ID != 0

:/ See the above comment, I think we instead need to look at Specified in some cases?

Copy link
Contributor Author

@otan otan 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 @ajwerner and @msbutler)


pkg/sql/sem/tree/tenant.go line 29 at r1 (raw file):

Previously, ajwerner wrote…

this comment is pretty stale now given we really actively rely on Specified and it's not "extra complexity"

i've removed the TODO


pkg/sql/sem/tree/tenant.go line 37 at r1 (raw file):

Previously, ajwerner wrote…

:/ See the above comment, I think we instead need to look at Specified in some cases?

i was just basing this on old logic (it fell to roachpb.TenantID.IsSet). Happy to chuck in a t.Specified here as well.

Refer to TenantID as uint64 instead.

Release note: None
Seems to rely on eval.Context behaviours, so moving it there.

Release note: None
* BytesNext now lives in the encoding package.
* tree now uses encoding.BytesNext instead of roachpb.Key.Next

Release note: None
@otan otan force-pushed the roachpb branch 2 times, most recently from c9f9070 to 80007da Compare April 28, 2022 12:57
@otan
Copy link
Contributor Author

otan commented Apr 28, 2022

had to drop the last commit for now

$ bazel query 'somepath(//pkg/sql/sem/tree:tree, //pkg/roachpb:roachpb)'
//pkg/sql/sem/tree:tree
//pkg/security:security
//pkg/clusterversion:clusterversion
//pkg/roachpb:roachpb

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 11 files at r5, 2 of 2 files at r6, 7 of 7 files at r7, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @msbutler)

Copy link
Contributor

@irfansharif irfansharif left a comment

Choose a reason for hiding this comment

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

Looks like the PR was able to break the dependency between pkg/sql/parser and pkg/roachpb; so we can remove this fellow:

"//pkg/roachpb", # keep

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @msbutler)

@otan
Copy link
Contributor Author

otan commented Apr 30, 2022

There is still a path to roachpb indirectly but Andrew is removing that.

@otan
Copy link
Contributor Author

otan commented May 1, 2022

bors r=ajwerner

@craig
Copy link
Contributor

craig bot commented May 1, 2022

Build failed:

@otan
Copy link
Contributor Author

otan commented May 1, 2022

bors r=ajwerner

@craig
Copy link
Contributor

craig bot commented May 2, 2022

Build succeeded:

@craig craig bot merged commit 810bd3a into cockroachdb:master May 2, 2022
craig bot pushed a commit that referenced this pull request May 2, 2022
80842: parser: remove roachpb from tree r=irfansharif a=otan

Reacting on an [earlier comment](#80684 (review)) which I misunderstood. Delete roachpb
from parser as it is no longer a dependency.

Refs: #79357

Release note: None

Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants