sql/distsqlplan: avoid excessive data copying#28942
Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom Aug 23, 2018
Merged
sql/distsqlplan: avoid excessive data copying#28942craig[bot] merged 2 commits intocockroachdb:masterfrom
craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
Member
|
This change is |
The functions to retrieve the sizes of value encodings have not been used in a long time. Remove them. Release note: None
The `ColumnType` struct is not trivially small. Also calling a by-pointer method on a copy may cause it to escape on the heap. Avoid making copies until necessary. Release note: None
f7b99e1 to
dbd7623
Compare
BramGruneir
requested changes
Aug 22, 2018
Member
BramGruneir
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained
pkg/sql/distsqlplan/physical_plan.go, line 891 at r2 (raw file):
merged[i] = *leftType } else { return nil, errors.Errorf("conflicting ColumnTypes: %v and %v", leftType, rightType)
shouldn't these also be referenced?
Also, is %v correct here?
And if ColumnType doesn't have a String() yet, it should. I think I hit upon the annoyance of this while debugging recently.
knz
commented
Aug 22, 2018
Contributor
Author
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlplan/physical_plan.go, line 891 at r2 (raw file):
Previously, BramGruneir (Bram Gruneir) wrote…
shouldn't these also be referenced?
Also, is %v correct here?
And if ColumnType doesn't have a String() yet, it should. I think I hit upon the annoyance of this while debugging recently.
It has already, it's implemented on the pointer type, so this fine.
BramGruneir
approved these changes
Aug 22, 2018
Member
BramGruneir
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status:
Contributor
Author
|
thank you! bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Aug 23, 2018
28942: sql/distsqlplan: avoid excessive data copying r=knz a=knz First commit from #28937. Needed for #28690. The `ColumnType` struct is not trivially small. Also calling a by-pointer method on a copy may cause it to escape on the heap. Avoid making copies until necessary. Release note: None Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Contributor
Build succeeded |
craig bot
pushed a commit
that referenced
this pull request
Aug 23, 2018
28943: sqlbase: add the missing comments and group functions by purpose r=knz a=knz First commits from #28942 and priors. Forked from #28690. The file `table.go` was getting unwieldy and was containing functions from different area of concern. This was making maintenance particularly burdensome. This patch splits the functionality in the following files: - `index_encoding.go`: index key prefixes, differences between primary and secondary encodings, how to organize columns in an index. - `column_type_encoding.go`: encoding of SQL values into bytes towards KV. - `column_type_properties.go`: conversions between various representations of types in the SQL layer; derivation of properties from SQL types. - `datum_alloc.go`: the `DatumAlloc` helper. Release note: None Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
craig bot
pushed a commit
that referenced
this pull request
Aug 23, 2018
29006: backport-2.1: fix the handling of SQL types r=knz a=knz Backport 1/1 commits from #28937. Backport 1/1 commits from #28942. Backport 17/17 commits from #28943. Backport 1/1 commits from #28944. Backport 1/1 commits from #28945. Backport 1/1 commits from #28949. Backport 2/2 commits from #28950. Backport 1/1 commits from #28951 (#28959). Backport 1/1 commits from #28952 (#28959). Backport 1/1 commits from #28955 (#28959). Backport 1/1 commits from #28959. Backport 1/1 commits from #28959. Backport 1/1 commits from #28690. cc @cockroachdb/release Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
craig bot
pushed a commit
that referenced
this pull request
Aug 24, 2018
29006: backport-2.1: fix the handling of SQL types r=knz a=knz Backport 1/1 commits from #28937. Backport 1/1 commits from #28942. Backport 17/17 commits from #28943. Backport 1/1 commits from #28944. Backport 1/1 commits from #28945. Backport 1/1 commits from #28949. Backport 2/2 commits from #28950. Backport 1/1 commits from #28951 (#28959). Backport 1/1 commits from #28952 (#28959). Backport 1/1 commits from #28955 (#28959). Backport 1/1 commits from #28959. Backport 1/1 commits from #28959. Backport 1/1 commits from #28690. cc @cockroachdb/release Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First commit from #28937.
Needed for #28690.
The
ColumnTypestruct is not trivially small. Also calling aby-pointer method on a copy may cause it to escape on the heap. Avoid
making copies until necessary.
Release note: None