sql: specify hash or merge join in EXPLAIN #35688
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 27 of 27 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @awoods187, @jordanlewis, @knz, and @RaduBerinde)
pkg/sql/walk.go, line 659 at r1 (raw file):
return "merge-join" } return "hash-join"
Is lookup join taken care of elsewhere?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/walk.go, line 659 at r1 (raw file):
Previously, rytaft wrote…
Is lookup join taken care of elsewhere?
Yes, that's a lookupJoinNode and it's already printed as lookup-join.
There was a problem hiding this comment.
@knz No need to review but let me know if you're ok with this change to replace join to hash-join or merge-join in EXPLAIN output.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/sql/walk.go, line 659 at r1 (raw file):
Previously, RaduBerinde wrote…
Yes, that's a
lookupJoinNodeand it's already printed aslookup-join.
👍
Replace `join` with `hash-join` or `merge-join` in `EXPLAIN` output. Fixes cockroachdb#35683. Release note (sql change): EXPLAIN tree now uses `hash-join` or `merge-join` instead of `join`.
RaduBerinde
force-pushed
the
explain-hash-merge
branch
from
fdec322 to
1832862
Compare
There was a problem hiding this comment.
Reviewable status:
|
bors r+ |
35688: sql: specify hash or merge join in EXPLAIN r=RaduBerinde a=RaduBerinde Replace `join` with `hash-join` or `merge-join` in `EXPLAIN` output. Fixes #35683. Release note (sql change): EXPLAIN tree now uses `hash-join` or `merge-join` instead of `join`. 35719: roachpb: prevent data race on Transaction r=nvanbenschoten a=nvanbenschoten Fixes #34241. This PR starts off with a series of small cleanups related to ownership of `roachpb.Transaction` objects and the use of deep and shallow clones. This makes up the first 5 commits. Next, the 6th commit removes redundant calls to `Transaction.UpdateObservedTimestamp`, reducing it down to have just a single caller that is easy to reason about. The 7th commit is what actually fixes the referenced issue. It adds in the proto clone that was missing from `BatchResponse_Header.combine` and allowing a `BatchRequest` to reference the same `Transaction` object as a `BatchResponse`. I confirmed a number of times that this stops the assertion from firing, so the commit also re-enables the skipped roachtest and removes the assertion. ~The final two commits are the two that we might consider waiting on and not including in this release, but they're also the most exciting. By making `Transaction.ObservedTimestamps` immutable (which it almost already was), we can prohibit all interior mutability of references within `Transaction`, give it value semantics, and eliminate the distinction between "shallow" and "deep" object cloning. This reduces the cost of a clone to a single straightforward allocation and makes working with the object easier to think about.~ EDIT: these two were removed from this PR. Past this point, I think the only other change we might want to make here is making a clone of `ba.Txn` in `internalClientAdapter` and then declare that the `Batch` handler goroutine has exclusive ownership over its `ba.Txn`. This would allow us to avoid a few conservative clones that would no longer be necessary, like [here](https://github.com/cockroachdb/cockroach/blob/57e825a7940495b67e0cc7213a5fabc24e12be0e/pkg/storage/store.go#L2827) and [here](https://github.com/cockroachdb/cockroach/blob/57e825a7940495b67e0cc7213a5fabc24e12be0e/pkg/storage/replica.go#L1309). I did not make this change here. 35736: opt: catch all pgerror.Error in optbuilder r=RaduBerinde a=RaduBerinde We now catch all `pgerror.Error`s in optbuilder, which means that we don't need to use the `buildError` wrapper with them. The wrapper still exists when external calls (e.g. for name resolution) return a generic error. The main motivation is that optbuilder calls into the factory which can panic internally. We will want to switch those panics to assertion errors as well, but they are in different packages. The existing approach would have required a shared, exported wrapper. Release note: None Co-authored-by: Radu Berinde <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]>
Build succeeded |
Replace
joinwithhash-joinormerge-joininEXPLAINoutput.Fixes #35683.
Release note (sql change): EXPLAIN tree now uses
hash-joinormerge-joininstead ofjoin.