colserde: add simple serialization for DECIMALs#43311
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Jan 14, 2020
Merged
colserde: add simple serialization for DECIMALs#43311craig[bot] merged 1 commit intocockroachdb:masterfrom
craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
Member
|
This change is |
b405409 to
ab2b238
Compare
yuzefovich
commented
Dec 19, 2019
Member
Author
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @asubiotto)
pkg/col/colserde/arrowbatchconverter_test.go, line 102 at r1 (raw file):
} } } else if typ == coltypes.Timestamp {
I've just run into a failure because require.Equal didn't work on equivalent timestamp slices. This probably happened because there is *time.Location in there, and maybe my assumption that those locations are cached (which I mentioned in pkg/sql/colexec/allocator.go:172) is incorrect. But it's a separate concern, so I'll leave it as TODO.
ab2b238 to
aa6a8e0
Compare
Member
Author
|
The benchmarks look rather sad though: |
aa6a8e0 to
8ae1809
Compare
Contributor
|
Thanks for this! Could you try running some higher-level benchmarks to see what the microbenchmark slowdown translates to when running a query? Maybe some distributed aggregation on decimals just above the row count threshold to get a worst case comparison. |
This commit adds simple serialization of DECIMALs using the provided MarshalText method. Release note (sql change): Previously, DECIMALs could not be sent over the network when the computation was performed by the vectorized engine. This has been fixed, and the vectorized engine now fully supports DECIMAL type.
Member
Author
|
I ran a simple benchmark as you suggested: I set up a three node local cluster and created a table with 1000 decimals which were split into 3 ranges with one range per node and then ran |
8ae1809 to
7bbd378
Compare
asubiotto
approved these changes
Jan 14, 2020
Contributor
asubiotto
left a comment
There was a problem hiding this comment.
Reviewed 5 of 6 files at r1, 2 of 2 files at r2.
Reviewable status:
Member
Author
|
TFTR! bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Jan 14, 2020
43311: colserde: add simple serialization for DECIMALs r=yuzefovich a=yuzefovich This commit adds simple serialization of DECIMALs using the provided MarshalText method. Touches: #37044. Release note (sql change): Previously, DECIMALs could not be sent over the network when the computation was performed by the vectorized engine. This has been fixed, and the vectorized engine now fully supports DECIMAL type. 43793: sql: properly enforce uniqueness of key columns for FK references r=lucy-zhang a=lucy-zhang We introduced a bug in 19.2 that would allow, e.g., a unique index on `(a, b, c)` to be used as an index that is supposed to enforce uniqueness for a foreign key constraint pointing only to `(a, b)`. This PR reintroduces a check that the indexed columns match the FK columns exactly. Fixes #42680. Release note (bug fix): Fixed a bug introduced in 19.2 that would allow foreign keys to use a unique index on the referenced columns that indexed more columns than were included in the columns used in the FK constraint, which allows potentially violating uniqueness in the referenced columns themselves. 43956: build: post issues from local roachtests r=andreimatei a=tbg This wasn't happening due to some missing env vars. I noticed this since there are many failures of acceptance/version-upgrade not reflected via created issues (I get email notifications from TC). Release note: None Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: Lucy Zhang <lucy-zhang@users.noreply.github.com> Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>
Contributor
Build succeeded |
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.
This commit adds simple serialization of DECIMALs using the provided
MarshalText method.
Touches: #37044.
Release note (sql change): Previously, DECIMALs could not be sent over
the network when the computation was performed by the vectorized engine.
This has been fixed, and the vectorized engine now fully supports
DECIMAL type.