Skip to content

execinfrapb: print input types one per line in EXPLAIN (DISTSQL, TYPES)#43409

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
yuzefovich:adjust-explain-distsql-types
Jan 14, 2020
Merged

execinfrapb: print input types one per line in EXPLAIN (DISTSQL, TYPES)#43409
craig[bot] merged 1 commit intocockroachdb:masterfrom
yuzefovich:adjust-explain-distsql-types

Conversation

@yuzefovich
Copy link
Copy Markdown
Member

@yuzefovich yuzefovich commented Dec 20, 2019

Previously, input types were printed in a single line within the input
synchronizer box on the flow diagram. However, when we have a processor
with two inputs and when each input has several columns, the boxes would
collide and make the types unreadable. This commit changes it to print
each type one per line, so there is no collision between different boxes
of input synchronizer. Separately, cockroachdb.github.io has been
adjusted so that the boxes for the input synchronizers could be
displayed well in such scenario.

Release note: None

@yuzefovich yuzefovich requested a review from a team as a code owner December 20, 2019 17:09
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@yuzefovich
Copy link
Copy Markdown
Member Author

Screen Shot 2019-12-20 at 9 08 14 AM

changes to

Screen Shot 2019-12-20 at 9 08 57 AM

Both options seem bad :/

@RaduBerinde
Copy link
Copy Markdown
Member

Ha, maybe spend a bit of time playing with https://github.com/cockroachdb/cockroachdb.github.io/blob/master/distsqlplan/flow_diagram.js to adjust the positioning of those things. I think this is where the "bounding box" is defined, the height should probably be proportional to the number of lines.

@yuzefovich yuzefovich force-pushed the adjust-explain-distsql-types branch from 4d7bc51 to 1f0477c Compare December 31, 2019 03:44
Previously, input types were printed in a single line within the input
synchronizer box on the flow diagram. However, when we have a processor
with two inputs and when each input has several columns, the boxes would
collide and make the types unreadable. This commit changes it to print
each type one per line, so there is no collision between different boxes
of input synchronizer. Separately, cockroachdb.github.io has been
adjusted so that the boxes for the input synchronizers could be
displayed well in such scenario.

Release note: None
@yuzefovich yuzefovich force-pushed the adjust-explain-distsql-types branch from 1f0477c to c932bc5 Compare December 31, 2019 18:13
@yuzefovich
Copy link
Copy Markdown
Member Author

Here is an example of what the plan looks like now (with the change to constraints):
Screen Shot 2020-01-14 at 10 16 37 AM
I think this PR should be good to go in.

@RaduBerinde
Copy link
Copy Markdown
Member

:lgtm: Consider adding a release note, someone might find it useful.

@yuzefovich
Copy link
Copy Markdown
Member Author

There was a release note in #43193, and I think it is sufficient (in a sense this PR is fixing what that PR started :) ).

TFTR!

bors r+

craig bot pushed a commit that referenced this pull request Jan 14, 2020
42845: coldata: expose a modifier for batchSize variable r=yuzefovich a=yuzefovich

This commit exposes a setter for batchSize private variable to be
modified in tests and runs all tests in sql/colexec with a random
batch size in [3, 4096] range.

The tests in several places needed to be adjusted because their
assumptions might no longer hold when batch size is modified.

Fixes: #40791.

Release note: None

43409: execinfrapb: print input types one per line in EXPLAIN (DISTSQL, TYPES) r=yuzefovich a=yuzefovich

Previously, input types were printed in a single line within the input
synchronizer box on the flow diagram. However, when we have a processor
with two inputs and when each input has several columns, the boxes would
collide and make the types unreadable. This commit changes it to print
each type one per line, so there is no collision between different boxes
of input synchronizer. Separately, cockroachdb.github.io has been
adjusted so that the boxes for the input synchronizers could be
displayed well in such scenario.

Release note: None

43887: pkg/sql/opt: validate correct use of FOR UPDATE locking clauses r=nvanbenschoten a=nvanbenschoten

The second commit updates our SQL grammar to be identical to Postgres regarding select statement locking clauses. Concretely, this results in the following changes in behavior:
- We can now parse multiple locking clause items
- We can now parse the `FOR READ ONLY` syntax, which is a no-op
- We can now parse the `SELECT ... FOR UPDATE ... LIMIT` syntax

Ref: https://github.com/postgres/postgres/blob/1a4a0329650b0545a54afb3c317aa289fd817f8a/src/backend/parser/gram.y#L11834

The third commit introduces a number of validation checks into the optbuilder to ensure that FOR UPDATE locking clauses are only being used in places where they're allowed. Most of this was based off of Postgres, which disallows the same set of operations to be used with `FOR [KEY] UPDATE/SHARE` locking.

Ref: https://github.com/postgres/postgres/blob/1a4a0329650b0545a54afb3c317aa289fd817f8a/src/backend/parser/analyze.c#L2691

Release note (sql change): Invalid usages of FOR UPDATE locking clauses are now rejected by the SQL optimizer.

43980: storage: rename SSTSnapshotStorage{,Scratch,File} vars r=ajwerner a=irfansharif

Around snapshot receiving code, we have variables for SSTSnapshotStorage, SSTSnapshotStorageScratch, and SSTSnapshotStorageFile types
all abbreviated to some variation of sss{,s,f}. We also have "ssts" to
talk about SSTs. This is all terribly confusing, we could do with some
much needed clarity here.

Release note: None.

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 14, 2020

Build succeeded

@craig craig bot merged commit c932bc5 into cockroachdb:master Jan 14, 2020
@yuzefovich yuzefovich deleted the adjust-explain-distsql-types branch January 14, 2020 22:36
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