colexec: fix an issue with builtin operators and a minor cleanup #43989
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
|
This change is |
yuzefovich
force-pushed
the
vec-fix-set-length
branch
4 times, most recently
from
4816332
to
911c22c
Compare
asubiotto
suggested changes
Jan 15, 2020
There was a problem hiding this comment.
Reviewed 14 of 14 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/colexec/and_or_projection_tmpl.go, line 187 at r1 (raw file):
// Run the right-side projection on the remaining tuples. o.rightFeedOp.batch = batch batch = o.rightProjOpChain.Next(ctx)
Would it be a possibility to copy this code above (before origLen > 0:
if origLen == 0 {
o.rightFeed ....
batch = o.rightProj....
return coldata.ZeroBatch
}
To not have to worry about an origLen == 0 in the rest of the code? Not sure if it can work or not.
pkg/sql/colexec/builtin_funcs.go, line 191 at r1 (raw file):
}, ) // Although we didn't change the length of the batch, it is beneficial to set
s/beneficial/necessary
Flat bytes relies on coldata.Batch.SetLength call to maintain its invariant. We assume that it is always called before return the batch in which Bytes vector might have been modified. This was not the case for default builtin and substring operators, and the calls were added. Additionally, to be safe, similar calls have been in added in projection operators. In a few places where we were setting the length of an internal batch to 0 and then returning it, those were replaced with returning coldata.ZeroBatch. This forced us to refactor CASE and AND/OR operators to handle cases of zero-length batches (previously, they assumed a well-typed zero-length batch which is not always the case). Release note: None
yuzefovich
force-pushed
the
vec-fix-set-length
branch
from
911c22c
to
c83e4b6
Compare
yuzefovich
commented
Jan 15, 2020
There was a problem hiding this comment.
Reviewable status:
pkg/sql/colexec/and_or_projection_tmpl.go, line 187 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Would it be a possibility to copy this code above (before
origLen > 0:if origLen == 0 { o.rightFeed .... batch = o.rightProj.... return coldata.ZeroBatch }To not have to worry about an
origLen == 0in the rest of the code? Not sure if it can work or not.
Good point. Fixed, but we have to return batch (see #44017).
pkg/sql/colexec/builtin_funcs.go, line 191 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
s/beneficial/necessary
Done.
asubiotto
approved these changes
Jan 15, 2020
There was a problem hiding this comment.
Reviewed 4 of 4 files at r2.
Reviewable status:
|
TFTR! bors r+ |
Build failed (retrying...) |
craig bot
pushed a commit
that referenced
this pull request
Jan 15, 2020
42969: storage: rationalize server-side refreshes and fix bugs r=andreimatei a=andreimatei Before this patch, we had several issues due to the server erroneously considering that it's OK to commit a transaction at a bumped timestamp. One of the issues was a lost update: a CPut could erroneously succeed even though there's been a more recent write. This was caused by faulty code in evaluateBatch() that was thinking that, just because an EndTxn claimed to have been able to commit a transaction, that means that any WriteTooOldError encountered previously by the batch was safe to discard. An EndTxn might consider that it can commit even if there had been previous write too old conditions if the NoRefreshSpans flag is set. The problems is that a CPut that had returned a WriteTooOldError also evaluated at the wrong read timestamp, and so its evaluation can't be relied on. Another issue is that, when the EndTxn code mentioned above considers that it's safe to commit at a bumped timestamp, it doesn't take into considerations that the EndTxn's batch might have performed reads (other than CPuts) that have been evaluated at a lower timestamp. This can happen, for example in the following scenario: - a txn sends a Put which gets bumped by the ts cache - the txn then sends a Scan + EndTxn. The scan gets evaluated at the original timestamp, but then we commit at a bumped one because the NoRefreshSpans flag is set. The patch fixes the bugs by reworking how evaluation takes advantage of the fact that some requests have flexible timestamps. EndTxn no longer is in the business of committing at bumped timestamps, and its code is thus simplified. Instead, the replica's "local retries" loop takes over. The replica already had code handling non-transactional batches that evaluated them repeatedly in case of WriteTooOldErrors. This patch rationalizes and expands this code to deal with transactional batches too, and with pushes besides WriteTooOldErrors. This reevaluation loop now handles the cases in which the EndTxn used to bump the commit timestamp. The patch also fixes a third bug: the logic evaluateBatch() for resetting the WriteTooOld state after a successful EndTransaction was ignoring the STAGING state, meaning that the server would return a WriteTooOldError even though the transaction was committed. I'm not sure if this had dramatic consequences or was benign... Fixes #42849 Release note (bug fix): A bug causing lost update transaction anomalies was fixed. 43915: cli: warn if trying to [rd]ecommision when already [rd]ecommissioned r=tbg a=knz Fixes #36624. First commit from #43908 Release note (cli change): The CLI commands `cockroach node decommission` and `cockroach node recommission` now produce a warning on the standard error if one of the node(s) specified is already (d/r)ecommissioned. 43989: colexec: fix an issue with builtin operators and a minor cleanup r=yuzefovich a=yuzefovich Flat bytes relies on coldata.Batch.SetLength call to maintain its invariant. We assume that it is always called before return the batch in which Bytes vector might have been modified. This was not the case for default builtin and substring operators, and the calls were added. Additionally, to be safe, similar calls have been in added in projection operators. In a few places where we were setting the length of an internal batch to 0 and then returning it, those were replaced with returning coldata.ZeroBatch. Fixes: #43656. Release note: None 44004: githooks: accept release note category 'security update' r=knz a=knz Forgot this in #43869 44008: re-enable: roachprod: Make multiple set [provider]-zones always geo-distribute nodes r=jlinder a=jlinder This re-enables commit d24e40e which was reverted in commit 63279f9. It was reverted due to roachtest automatically passing in a list of zones but only wanting the first zone to be used (#43898) which was fixed in f68c6d5 . Before: if multiple zones were set for a provider and --geo wasn't set, all hosts would be started in just one zone in one region. Why change? Because if multiple zones are set, the intention is that they be used. Now, --geo and --[provider]-zones work as follows for gcloud, aws and azure: 1. when geo and zones are not set, nodes are all placed in one of the default zones 2. when geo is set but zones aren't, nodes are spread evenly across the default zones 3. when zones are set, nodes are spread evenly across the specified zones Fixes #38542. Release note: None 44016: builtins: miscellaneous fixes for the to_hex builtin r=mjibson a=otan Resolves #41707. Release note (sql change, bug fix): * Added to_hex(string) -> string functionality. * Previously, `to_hex(-1)` would return `-1` instead of the negative hex representation (`FFFFFFFFFFFFFFFF`). This has been rectified in this PR. 44023: roachtest: bump minimum version of the sqlsmith roachtest r=yuzefovich a=rohany Fixes #43995. This PR bumps the minimum version of the SQLSmith roachtest to be v20.1.0. Release note: None Co-authored-by: Andrei Matei <andrei@cockroachlabs.com> Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: James H. Linder <jamesl@cockroachlabs.com> Co-authored-by: Oliver Tan <otan@cockroachlabs.com> Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
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.
Flat bytes relies on coldata.Batch.SetLength call to maintain its
invariant. We assume that it is always called before return the batch in
which Bytes vector might have been modified. This was not the case for
default builtin and substring operators, and the calls were added.
Additionally, to be safe, similar calls have been in added in projection
operators.
In a few places where we were setting the length of an internal batch to
0 and then returning it, those were replaced with returning
coldata.ZeroBatch.
Fixes: #43656.
Release note: None