distsql: use slices for populating output of windowers #28341
Merged
Conversation
This file contains 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
Using slices instead of maps gives significant performance improvement. On several quiries I ran with profiling, time spent while populating output rows reduced from 10s or to 1-2s (in terms of performance degradation: the difference went down from 90-100% to about 20%). Related to: cockroachdb#24425. Release note: None
|
This change is |
jordanlewis
approved these changes
Aug 7, 2018
There was a problem hiding this comment.
LGTM, nice find
Reviewed 1 of 1 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained
|
Thanks for quick review! |
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Aug 7, 2018
27978: sql: improve handling of decimal zeroes r=mjibson a=mjibson Previously we had inconsistent handling of decimal zeroes. Various forms (-0, -0.00, +0.00) were correctly handled from a "what is this equal to" perspective, but we were not consistent in keeping the digits after the decimal point. Furthermore, postgres always removes negatives when making -0 for ints and decimals (not floats). This behavior (removing the negative from -0 automatically) existed in the higher level apd tests but we were subverting it by using apd.Decimal.Neg directly. apd has been updated to do this for Decimals now. Our constant evaluation has also caused us some problems. We use the go/constant package, but it evaluates numbers as floats. When we applied the + operator our NumVal type would remove its OrigString representation and thus use only the constant's Value, which, as a float, didn't contain the trailing digits after the decimal (in cases like +0.000). Now the + operator is a pass through. The - operator remains the same, but has special logic to detect -0 and leave the original string The added logic tests have been compared to postgres and now we match them exactly for many more things. Of note is that, in postgres, `SELECT -0::float` returns -0 (in opposition to the decimal behavior). This is why the test in TestParseDatumStringAs removed only the -0 test for decimals and not floats. Release note (sql change): More correct handling of decimal 0s. Specifically, -0 is coerced to 0 and values like 0.00 retain the digits after the decimal point. 28145: sql: hopefully reduce the memory usage of TestDropWhileBackfill r=vivekmenezes a=vivekmenezes related to #28084 Release note: None 28229: rangefeed: add resolved timestamp initialization scan and registration catch-up scan r=nvanbenschoten a=nvanbenschoten This PR includes a number of small improvements to the `rangefeed` package, culminating in the introduction of two scans that a rangefeed will need to perform. The first is a scan over the entire key range from the previously known resolved timestamp onward to learn about all existing unresolved intents. The second is a scan over a new registration's key range to backfill in all values from before it was registered but after its starting timestamp. Both of these scans will be able to take advantage of TimeBound iterators. However, the way they're represented in the `rangefeed` package is pretty abstract, which made testing pretty easy. The first commit and the last two are substantial, but the rest are just small logical changes so I hope this is easy to review. @benesch thanks for offering to take a look at this while @tschottdorf is out! There's only been one PR merged so far so it shouldn't take much to catch up to speed: #28072. 28313: workload: revert --init idempotency if initial rows are loaded r=nvanbenschoten a=nvanbenschoten Fixes #27981. Release note: None 28326: cli: set a timeout delay for SQL connections r=knz a=knz Fixes #20097. This patch leverages a feature in the underlying SQL driver `lib/pq` to set a connection timeout for all CLI commands that use a SQL connection. This includes `cockroach sql`/`demo`, `cockroach node ls`, `cockroach zone set`, etc. The timeout is set to 5 seconds. If the client cannot connect within 5 seconds it fails with the following error: ``` Error: cannot dial server. Is the server running? If the server is running, check --host client-side and --advertise server-side. read tcp [::1]:60205->[::1]:26257: i/o timeout ``` Release note (cli change): the various client commands that use a SQL connection (including e.g. `cockroach sql`, `cockroach node`, `cockroach user`) now produce an error if a connection could not be established within 5 seconds instead of waiting forever. 28341: distsql: use slices for populating output of windowers r=yuzefovich a=yuzefovich Using slices instead of maps gives significant performance improvement. On several quiries I ran with profiling, time spent while populating output rows reduced from 10s or to 1-2s (in terms of performance degradation: the difference went down from 90-100% to about 20%). Related to: #24425. Release note: None Co-authored-by: Matt Jibson <[email protected]> Co-authored-by: Vivek Menezes <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]> Co-authored-by: yuzefovich <[email protected]>
Build succeeded |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Using slices instead of maps gives significant
performance improvement. On several quiries I
ran with profiling, time spent while populating
output rows reduced from 10s or to 1-2s (in terms
of performance degradation: the difference went
down from 90-100% to about 20%).
Related to: #24425.
Release note: None