-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
opt: explain doesn't show hash or merge join explicitly #35683
Comments
Lookup has the ideal ux:
While merge does not. You can figure it out in the next column, but it would be better to see it in the first like lookup.
|
Also, for explain (opt, verbose) we should match hash to merge and lookup:
|
The EXPLAIN OPT output is currently mostly for our benefit (when debugging) so I'm not crazy about considering "usability" improvements. The output closely mirrors the internal structure of our expressions (which is subject to change): a The regular EXPLAIN output has been like this forever, but I agree it can be improved. I don't see a problem with renaming |
I think we definitely should fix Can we match |
Yes. If users seem to need I edited the title because this doesn't have much to do with hints as far as I can tell. |
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`.
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`.
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]>
Describe the problem
Explain does not show that a hash join has occured:
Explain (opt) does show hash:
Explain (opt, verbose) does as well:
The text was updated successfully, but these errors were encountered: