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: move AsOfSystemTime to EvalContext #68219

Merged
merged 2 commits into from
Jul 30, 2021

Conversation

otan
Copy link
Contributor

@otan otan commented Jul 29, 2021

Previously, AsOfSystemTime was on the SemaCtx. With the bounded
staleness work needing this in the EvalContext, and all other contexts
accessing this variable have access to EvalContext, move
AsOfSystemTime from SemaContext to EvalContext.

Release note: None

@otan otan added the do-not-merge bors won't merge a PR with this label. label Jul 29, 2021
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rytaft
Copy link
Collaborator

rytaft commented Jul 29, 2021

I think this was a good idea! I canceled bors-ing my other PR so feel free to re-open this.

@otan otan changed the title [WIP] sql: move AsOfSystemTime to EvalContext sql: move AsOfSystemTime to EvalContext Jul 29, 2021
@otan otan removed the do-not-merge bors won't merge a PR with this label. label Jul 29, 2021
@otan otan requested a review from rytaft July 29, 2021 12:45
@otan otan reopened this Jul 29, 2021
@otan
Copy link
Contributor Author

otan commented Jul 29, 2021

feel free to bors this if you accept @rytaft :D im off to bed!

@otan otan marked this pull request as ready for review July 29, 2021 12:45
Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @otan)


pkg/sql/conn_executor.go, line 2408 at r2 (raw file):

	p.semaCtx.SearchPath = ex.sessionData.SearchPath
	p.semaCtx.IntervalStyleEnabled = ex.sessionData.IntervalStyleEnabled
	p.semaCtx.AsOfSystemTime = nil

if you're not going to reset it here, seems like you should actually remove it from the SemaContext, no? I think there are still some references to this field. For example:

p.SemaCtx().AsOfSystemTime = &tree.AsOfSystemTime{Timestamp: *details.AsOf}

otan added 2 commits July 30, 2021 07:12
Previously, AsOfSystemTime was on the SemaCtx. With the bounded
staleness work needing this in the EvalContext, and all other contexts
accessing this variable have access to EvalContext, move
AsOfSystemTime from SemaContext to EvalContext.

Release note: None
This reverts commit 25b4d57. No longer
needed as AsOfSystemTime is moved to EvalContext.

Release note: None
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.

revert also included

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


pkg/sql/conn_executor.go, line 2408 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

if you're not going to reset it here, seems like you should actually remove it from the SemaContext, no? I think there are still some references to this field. For example:

p.SemaCtx().AsOfSystemTime = &tree.AsOfSystemTime{Timestamp: *details.AsOf}

urgh, knew it was too good to be true to pass first go
removed!

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

:lgtm: Thanks!!

Reviewed 5 of 5 files at r3, 10 of 10 files at r4.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @otan)

@otan
Copy link
Contributor Author

otan commented Jul 30, 2021

bors r=rytaft

cheers

@craig
Copy link
Contributor

craig bot commented Jul 30, 2021

Build succeeded:

@craig craig bot merged commit e20731d into cockroachdb:master Jul 30, 2021
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.

3 participants