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

docgen: update SQL diagrams for SQL docs refactor #95591

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

taroface
Copy link
Collaborator

@taroface taroface commented Jan 20, 2023

Updated existing & added some new SQL diagrams. These will be used in the SQL docs being refactored as per https://cockroachlabs.atlassian.net/browse/DOC-5850

Epic: none

Release note: none

Release justification: non-production code change

@taroface taroface requested review from a team as code owners January 20, 2023 17:46
@blathers-crl
Copy link

blathers-crl bot commented Jan 20, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

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.

Parser changes :lgtm:, but I have a question about the diagrams change

Reviewed 19 of 19 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @taroface)


pkg/cmd/docgen/diagrams.go line 403 at r1 (raw file):

	},
	{
		name:	"alter_database",

I'm not familiar with this file or how to review changes here. Is there something I can look at?

@taroface
Copy link
Collaborator Author

taroface commented Jan 20, 2023

Parser changes :lgtm:, but I have a question about the diagrams change

Reviewed 19 of 19 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @taroface)

pkg/cmd/docgen/diagrams.go line 403 at r1 (raw file):

	},
	{
		name:	"alter_database",

I'm not familiar with this file or how to review changes here. Is there something I can look at?

Yes, the results are in alter_database.bnf in this PR. I previewed it by plugging it into https://www.bottlecaps.de/rr/ui

@taroface
Copy link
Collaborator Author

Added the backport-22.2.x label hoping this will also generate the appropriate files in the 22.2 branch for me to embed in docs.

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:

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


pkg/cmd/docgen/diagrams.go line 403 at r1 (raw file):

Previously, taroface (Ryan Kuo) wrote…

Yes, the results are in alter_database.bnf in this PR. I previewed it by plugging it into https://www.bottlecaps.de/rr/ui

Thanks!

@taroface
Copy link
Collaborator Author

bors r=rytaft

@craig
Copy link
Contributor

craig bot commented Jan 20, 2023

Build failed (retrying...):

@renatolabs
Copy link
Contributor

bors r-

SeeBazel Essential CI failures.

@craig
Copy link
Contributor

craig bot commented Jan 20, 2023

Canceled.

@taroface
Copy link
Collaborator Author

bors r=rytaft

@craig
Copy link
Contributor

craig bot commented Jan 23, 2023

Build succeeded:

@craig craig bot merged commit 1a4bcbf into cockroachdb:master Jan 23, 2023
@blathers-crl
Copy link

blathers-crl bot commented Jan 23, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from bd7c9b9 to blathers/backport-release-22.2-95591: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@taroface taroface deleted the sql-params branch January 24, 2023 20:37
craig bot pushed a commit that referenced this pull request Jan 27, 2023
95153: sql/schemachanger: Drop function in declarative schema changer r=chengxiong-ruan a=chengxiong-ruan

This pr contains 3 commits:
1. add function descriptor elements definition
2. a simple rename of RemoveViewBackReferencesInRelation to RemoveBackReferencesInRelation
3. drop function logics
Part of: https://cockroachlabs.atlassian.net/browse/CRDB-19495

95524: kvserver: fix race on Replica.tenantLimiter r=tbg a=pavelkalinnikov

When Replica is being destroyed, it can set tenantLimiter to nil. However, all accessors assume synchronization through IsInitialized atomic check and that this field can't be modified after the replica is initialized.

There is no particular need in nullifying this limiter, the replica is going away soon, so this commit removes this line in order to fix the data race.

Fixes #95345
Epic: none
Release note: none

96002: eval: fix internal error casting bytes to bit r=msirek a=msirek

Fixes #95543

Release note (bug fix): This patch fixes an internal error which may occur in the SHOW RANGE FROM TABLE statement when the FOR ROW clause specifies a BYTE literal and the corresponding column data type is BIT.

96030: docgen: remove redundant SQL diagrams r=nickvigilante a=taroface

Diagrams updated in #95591 were not generating HTML files as intended because of some redundant lines in the code. This removes the redundancies per `@nickvigilante.`

Epic: none

Release note: none

Release justification: non-production code change

96048: optbuilder: fix internal error in LATERAL queries with redundant function calls r=DrewKimball,mgartner a=msirek

Fixes #95615

Function `optbuilder.Builder.buildZip` may try to build 2 identical function
calls into 2 zip expressions, each one adding one or more columns to the
output. However, if buildZip's called to `buildScalar` recognizes a scalar
expression has already been built, it skips adding a new output columns and
returns the previously-built output column. This mismatch in the number of
output columns actually built, and the number of columns communicated to
the parent expression via `outScope` confuses the execution engine, and in
this case we error out because the expected data type of one column doesn't
match the actual data type.

The fix is to skip building zip expressions with redundant expressions which
already exist in `outScope`.

Release note (bug fix): This patch fixes rare internal errors in LATERAL
queries with redundant function calls.

96099: upgrades: share a column family for the new tenant columns r=ajwerner a=knz

Fixes #96093.

The previous change in that area was causing the new columns to be (implicitly) added into separate column families. However, we're expecting (from the bootstrap schema) to use a single column family for them. This patch fixes that.

Release note: None

Co-authored-by: Chengxiong Ruan <[email protected]>
Co-authored-by: Pavel Kalinnikov <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Mark Sirek <[email protected]>
Co-authored-by: Ryan Kuo <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
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