opt: do not add invalid casts to match types in set operations #75219
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
The optbuilder adds casts in order to match non-identical left/right types of set operations like UNION (see cockroachdb#60560). This change prevents the optbuilder from adding casts that are invalid, which caused internal errors. Now, a user error is thrown if no such cast exists from the left or right input type to the output type. Release note (bug fix): A bug has been fixed that caused internal errors in queries with set operations, like UNION, when corresponding columns on either side of the set operation were not the same. This error only occurred with a limited set of types. This bug is present in versions 20.2.6+, 21.1.0+, and 21.2.0+.
|
This change is |
RaduBerinde
approved these changes
Jan 21, 2022
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @rafiss)
|
TFTR! bors r+ |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from f46f377 to blathers/backport-release-21.1-75219: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 21.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
mgartner
added a commit
to mgartner/cockroach
that referenced
this pull request
Feb 7, 2022
The optbuilder no longer creates invalid casts when building CASE expressions that have branches of different types. CASE expressions that previously caused internal errors now result in user-facing errors. A similar change was made recently to UNION expressions in cockroachdb#75219. Note that there is more work to be done to be fully consistent with Postgres's CASE typing behavior, see cockroachdb#75365. Fixes cockroachdb#75365 Release note (bug fix): CASE expressions with branches that result in types that cannot be cast to a common type now result in a user-facing error instead of an internal error.
craig bot
pushed a commit
that referenced
this pull request
Feb 8, 2022
…76205 75541: rfc: kv lock observability via `crdb_locks` r=AlexTalks a=AlexTalks _Link to [RFC text](https://github.com/AlexTalks/cockroach/blob/rfc_crdb_locks/docs/RFCS/20220104_crdb_locks.md)._ This RFC proposes a feature for observing locks (and contention on locks) via a new virtual table called `crdb_internal.crdb_locks`. Release note: None 75738: roachtest/follower_reads: unskip follower-reads/mixed-version/single-region r=andreimatei a=andreimatei It should have been unskipped with the rest of the follower reads tests when #69817 was closed. Release note: None 75898: sql: consider virtual PK columns as stored in the optimizer r=mgartner a=mgartner Allowing virtual primary key columns created a contradiction of two hard-coded assumptions of the optimizer: 1. Primary key columns are written to the primary index (and all secondary indexes). 2. Virtual columns are not written to the primary index. To prevent this contradiction from causing bugs while planning, the opt catalog presents columns that are marked as virtual PK columns in the descriptor as stored columns to the optimizer. This is valid because virtual computed PK columns behave exactly like stored computed PK columns: they are written to all indexes. The test catalog has been updated to mimic this behavior. Fixes #75147 Fixes #75874 Release note (bug fix): A bug has been fixed that caused internal errors when querying tables with virtual columns in the primary key. This bug was only present since version 22.1.0-alpha.1 and does not appear in any production releases. 75912: util/tracing: don't return recordings for non-recording spans r=andreimatei a=andreimatei This patch makes sp.GetRecording() return nil if the span was not recording. This is already what the documentation on GetRecording() was implying, but the actual behavior was different: a non-recording span (that's not a no-op span) would return a non-nil recording consisting only of the the span's metadata. This behavior is not necessarily unreasonable, but it turns out it's a bit dangerous from a performance perspective - there's a cost (e.g. some allocations) for generating this recording, and it was too easy for unsuspecting callers to pay that when they didn't want to. In particular, DistSQL was indiscriminently calling GetRecording() (through its GetTraceData* utilities), and paying this measurable cost unintentionally. Eliminating this cost is particularly important when the Tracer is configured in the "active spans registry" tracing mode; when not in that mode, all the spans would be no-ops and the GetRecording calls were shortcircuited anyway. This is seen in BenchmarkTracing//trace=on, particularly on "3node/scan" which was the benchmark with the bigest difference between tracing on and off. name old time/op new time/op delta Tracing/1node/scan/trace=on-32 214µs ± 4% 206µs ± 5% ~ (p=0.222 n=5+5) Tracing/1node/insert/trace=on-32 338µs ± 5% 334µs ± 4% ~ (p=0.690 n=5+5) Tracing/3node/scan/trace=on-32 515µs ± 3% 484µs ± 3% -6.11% (p=0.008 n=5+5) Tracing/3node/insert/trace=on-32 635µs ± 1% 627µs ± 1% ~ (p=0.095 n=5+5) name old alloc/op new alloc/op delta Tracing/1node/scan/trace=on-32 27.2kB ± 2% 25.6kB ± 2% -5.94% (p=0.008 n=5+5) Tracing/1node/insert/trace=on-32 45.2kB ± 2% 45.5kB ± 2% ~ (p=0.690 n=5+5) Tracing/3node/scan/trace=on-32 83.2kB ± 3% 79.7kB ± 5% ~ (p=0.095 n=5+5) Tracing/3node/insert/trace=on-32 102kB ± 2% 103kB ± 1% ~ (p=0.548 n=5+5) name old allocs/op new allocs/op delta Tracing/1node/scan/trace=on-32 242 ± 1% 229 ± 0% -5.37% (p=0.000 n=5+4) Tracing/1node/insert/trace=on-32 338 ± 1% 339 ± 1% ~ (p=0.563 n=5+5) Tracing/3node/scan/trace=on-32 819 ± 1% 788 ± 6% ~ (p=0.135 n=5+5) Tracing/3node/insert/trace=on-32 871 ± 0% 880 ± 0% +0.99% (p=0.016 n=4+5) Release note: None 75981: kvserver: clean-up the interaction between the mergeQueue and `AdminRelocateRange` r=aayushshah15 a=aayushshah15 This commit makes it such that the `mergeQueue` no longer has to take special care to avoid a lease transfer when calling into `AdminRelocateRange`. This was enabled by #75676. Release note: None 75999: kvserver: stop reconciling lease count imbalances in the `StoreRebalancer` r=aayushshah15 a=aayushshah15 Generally, the `StoreRebalancer` doesn't try to attempt lease transfers for replicas that account for less than 0.1% of the store's total QPS. However, it allows such lease transfers if the store has a higher than average number of leases. This is fraught because of the following reasons: 1. It doesn't adhere to any padding. It will do these tiny lease transfers until the store has less than or equal to the average number of leases. 2. Additionally, "average" here means the average across the cluster, not just the stores that have replicas for the range. This is clearly not good, as this doesn't translate well to heterogeneously loaded clusters. 3. These lease transfers also don't adhere to `kv.allocator.min_lease_transfer_interval`. This means they are not rate limited like the lease transfers done by the replicateQueue. This patch fixes this behavior. This patch is similar to #64559. Noticed while running a tpcc workload on a multi-region cluster, but on a database constrained to a single region. This was done to simulate a "heterogeneously loaded cluster". Release note: None 76078: opt: plan inner lookup joins on virtual column indexes in more cases r=mgartner a=mgartner ExtractJoinEqualities now reuses computed columns instead of synthesizing new columns when it creates projections that exactly match a computed column expression of a base table. This allows GenerateLookupJoinsWithVirtualCols to generate lookup joins in more cases. This also paves the way for exploring anti- and semi-lookup joins on indexes with virtual columns and expression indexes. Fixes #75872 Release note (performance improvement): The optimizer now plans inner lookup joins using expression indexes in more cases, resulting in more efficient query plans. 76193: optbuilder: do not create invalid casts when building CASE expressions r=mgartner a=mgartner The optbuilder no longer creates invalid casts when building CASE expressions that have branches of different types. CASE expressions that previously caused internal errors now result in user-facing errors. A similar change was made recently to UNION expressions in #75219. Note that there is more work to be done to be fully consistent with Postgres's CASE typing behavior, see #75365. Fixes #75365 Release note (bug fix): CASE expressions with branches that result in types that cannot be cast to a common type now result in a user-facing error instead of an internal error. 76200: cloud: bump orchestrator to v21.2.5 r=celiala a=Azhng Release note: None 76205: roachtest: update 22.1 version map to v21.2.5 r=celiala a=Azhng Release note: None Co-authored-by: Alex Sarkesian <sarkesian@cockroachlabs.com> Co-authored-by: Andrei Matei <andrei@cockroachlabs.com> Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com> Co-authored-by: Aayush Shah <aayush.shah15@gmail.com> Co-authored-by: Azhng <archer.xn@gmail.com>
mgartner
added a commit
to mgartner/cockroach
that referenced
this pull request
Feb 15, 2022
The optbuilder no longer creates invalid casts when building CASE expressions that have branches of different types. CASE expressions that previously caused internal errors now result in user-facing errors. A similar change was made recently to UNION expressions in cockroachdb#75219. Note that there is more work to be done to be fully consistent with Postgres's CASE typing behavior, see cockroachdb#75365. Fixes cockroachdb#75365 Release note (bug fix): CASE expressions with branches that result in types that cannot be cast to a common type now result in a user-facing error instead of an internal error.
This was referenced Feb 15, 2022
mgartner
added a commit
to mgartner/cockroach
that referenced
this pull request
Feb 22, 2022
The optbuilder no longer creates invalid casts when building CASE expressions that have branches of different types. CASE expressions that previously caused internal errors now result in user-facing errors. A similar change was made recently to UNION expressions in cockroachdb#75219. Note that there is more work to be done to be fully consistent with Postgres's CASE typing behavior, see cockroachdb#75365. Fixes cockroachdb#75365 Release note (bug fix): CASE expressions with branches that result in types that cannot be cast to a common type now result in a user-facing error instead of an internal error.
mgartner
added a commit
to mgartner/cockroach
that referenced
this pull request
Feb 22, 2022
The optbuilder no longer creates invalid casts when building CASE expressions that have branches of different types. CASE expressions that previously caused internal errors now result in user-facing errors. A similar change was made recently to UNION expressions in cockroachdb#75219. Note that there is more work to be done to be fully consistent with Postgres's CASE typing behavior, see cockroachdb#75365. Fixes cockroachdb#75365 Release note (bug fix): CASE expressions with branches that result in types that cannot be cast to a common type now result in a user-facing error instead of an internal error.
RajivTS
pushed a commit
to RajivTS/cockroach
that referenced
this pull request
Mar 6, 2022
The optbuilder no longer creates invalid casts when building CASE expressions that have branches of different types. CASE expressions that previously caused internal errors now result in user-facing errors. A similar change was made recently to UNION expressions in cockroachdb#75219. Note that there is more work to be done to be fully consistent with Postgres's CASE typing behavior, see cockroachdb#75365. Fixes cockroachdb#75365 Release note (bug fix): CASE expressions with branches that result in types that cannot be cast to a common type now result in a user-facing error instead of an internal error.
mgartner
added a commit
to mgartner/cockroach
that referenced
this pull request
Mar 10, 2022
The optbuilder no longer creates invalid casts when building COALESCE and IF expressions that have children with different types. Expressions that previously caused internal errors now result in user-facing errors. Both UNION and CASE expressions had similar bugs that were recently fixed in cockroachdb#75219 and cockroachdb#76193. This commit also updates the `tree.ReType` function to return `ok=false` if there is no valid cast to re-type the expression to the given type. This forces callers to explicitly deal with situations where re-typing is not possible and it ensures that the function never creates invalid casts. This will make it easier to track down future related bugs because internal errors should originate from the call site of `tree.ReType` rather than from logic further along in the optimization process (in the case of cockroachdb#76807 the internal error originated from the logical props builder when it attempted to lookup the volatility of the invalid cast). Fixes cockroachdb#76807 Release justification: This is a low-risk change that fixes a minor bug. Release note (bug fix): A bug has been fixed that caused internal errors when COALESCE and IF expressions had inner expressions with different types that could not be cast to a common type.
mgartner
added a commit
to mgartner/cockroach
that referenced
this pull request
Mar 10, 2022
The optbuilder no longer creates invalid casts when building COALESCE and IF expressions that have children with different types. Expressions that previously caused internal errors now result in user-facing errors. Both UNION and CASE expressions had similar bugs that were recently fixed in cockroachdb#75219 and cockroachdb#76193. This commit also updates the `tree.ReType` function to return `ok=false` if there is no valid cast to re-type the expression to the given type. This forces callers to explicitly deal with situations where re-typing is not possible and it ensures that the function never creates invalid casts. This will make it easier to track down future related bugs because internal errors should originate from the call site of `tree.ReType` rather than from logic further along in the optimization process (in the case of cockroachdb#76807 the internal error originated from the logical props builder when it attempted to lookup the volatility of the invalid cast). Fixes cockroachdb#76807 Release justification: This is a low-risk change that fixes a minor bug. Release note (bug fix): A bug has been fixed that caused internal errors when COALESCE and IF expressions had inner expressions with different types that could not be cast to a common type.
mgartner
added a commit
to mgartner/cockroach
that referenced
this pull request
Mar 11, 2022
The optbuilder no longer creates invalid casts when building COALESCE and IF expressions that have children with different types. Expressions that previously caused internal errors now result in user-facing errors. Both UNION and CASE expressions had similar bugs that were recently fixed in cockroachdb#75219 and cockroachdb#76193. This commit also updates the `tree.ReType` function to return `ok=false` if there is no valid cast to re-type the expression to the given type. This forces callers to explicitly deal with situations where re-typing is not possible and it ensures that the function never creates invalid casts. This will make it easier to track down future related bugs because internal errors should originate from the call site of `tree.ReType` rather than from logic further along in the optimization process (in the case of cockroachdb#76807 the internal error originated from the logical props builder when it attempted to lookup the volatility of the invalid cast). This commit also adds special logic to make casts from any tuple type to `types.AnyTuple` valid immutable, implicit casts. Evaluation of these casts are no-ops. Users cannot construct these casts, but they are built by optbuilder in some cases. Fixes cockroachdb#76807 Release justification: This is a low-risk change that fixes a minor bug. Release note (bug fix): A bug has been fixed that caused internal errors when COALESCE and IF expressions had inner expressions with different types that could not be cast to a common type.
mgartner
added a commit
to mgartner/cockroach
that referenced
this pull request
Mar 11, 2022
The optbuilder no longer creates invalid casts when building COALESCE and IF expressions that have children with different types. Expressions that previously caused internal errors now result in user-facing errors. Both UNION and CASE expressions had similar bugs that were recently fixed in cockroachdb#75219 and cockroachdb#76193. This commit also updates the `tree.ReType` function to return `ok=false` if there is no valid cast to re-type the expression to the given type. This forces callers to explicitly deal with situations where re-typing is not possible and it ensures that the function never creates invalid casts. This will make it easier to track down future related bugs because internal errors should originate from the call site of `tree.ReType` rather than from logic further along in the optimization process (in the case of cockroachdb#76807 the internal error originated from the logical props builder when it attempted to lookup the volatility of the invalid cast). This commit also adds special logic to make casts from any tuple type to `types.AnyTuple` valid immutable, implicit casts. Evaluation of these casts are no-ops. Users cannot construct these casts, but they are built by optbuilder in some cases. Fixes cockroachdb#76807 Release justification: This is a low-risk change that fixes a minor bug. Release note (bug fix): A bug has been fixed that caused internal errors when COALESCE and IF expressions had inner expressions with different types that could not be cast to a common type.
craig bot
pushed a commit
that referenced
this pull request
Mar 11, 2022
77349: spanconfig: add Record constructor and validation r=adityamaru a=adityamaru This change adds a constructor method that returns a new `spanconfig.Record` and conditionally performs some validation if the target is a SystemTarget. Informs: #73727 Release note: None Release justification: low-risk updates to new functionality 77608: optbuilder: do not create invalid casts when building COALESCE and IF r=mgartner a=mgartner The optbuilder no longer creates invalid casts when building COALESCE and IF expressions that have children with different types. Expressions that previously caused internal errors now result in user-facing errors. Both UNION and CASE expressions had similar bugs that were recently fixed in #75219 and #76193. This commit also updates the `tree.ReType` function to return `ok=false` if there is no valid cast to re-type the expression to the given type. This forces callers to explicitly deal with situations where re-typing is not possible and it ensures that the function never creates invalid casts. This will make it easier to track down future related bugs because internal errors should originate from the call site of `tree.ReType` rather than from logic further along in the optimization process (in the case of #76807 the internal error originated from the logical props builder when it attempted to lookup the volatility of the invalid cast). This commit also adds special logic to make casts from any tuple type to `types.AnyTuple` valid immutable, implicit casts. Evaluation of these casts are no-ops. Users cannot construct these casts, but they are built by optbuilder in some cases. Fixes #76807 Release justification: This is a low-risk change that fixes a minor bug. Release note (bug fix): A bug has been fixed that caused internal errors when COALESCE and IF expressions had inner expressions with different types that could not be cast to a common type. 77632: ui: new plan table on statement details r=maryliag a=maryliag Previously, the Explain Plan tab on Statement Details was showing only one plan. This commit introduces a table of plan with their respective executions stats. When a plan is clicked on the table, it shows the Plan and its statistics. Fixes #72129 Page on DB Console: [video](https://www.loom.com/share/0898f48021eb4037a6f86760053a5e85) Page on CC Console: [video](https://www.loom.com/share/84d8ec40ae7e4eb19291788721ab7133) <img width="1058" alt="Screen Shot 2022-03-10 at 2 42 25 PM" src="https://user-images.githubusercontent.com/1017486/157742210-12b79f5d-274c-48e6-8fb6-dafc74fd25b3.png"> <img width="988" alt="Screen Shot 2022-03-10 at 2 42 36 PM" src="https://user-images.githubusercontent.com/1017486/157742211-daa2a07c-b025-4b36-a49a-4cafe7117bc8.png"> Release justification: Category 4 Release note (ui change): Explain Plan tab on Statement Details shows statistics for all the plans executed by the selected statement on the selected period. 77688: ci: ensure all nightlies post github issues when tests fail r=rail a=rickystewart Release justification: ensure failing nightlies post github issues Release note: None Co-authored-by: Aditya Maru <adityamaru@gmail.com> Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com> Co-authored-by: Marylia Gutierrez <marylia@cockroachlabs.com> Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>
This was referenced Mar 23, 2022
mgartner
added a commit
to mgartner/cockroach
that referenced
this pull request
Mar 23, 2022
The optbuilder no longer creates invalid casts when building COALESCE and IF expressions that have children with different types. Expressions that previously caused internal errors now result in user-facing errors. Both UNION and CASE expressions had similar bugs that were recently fixed in cockroachdb#75219 and cockroachdb#76193. This commit also updates the `tree.ReType` function to return `ok=false` if there is no valid cast to re-type the expression to the given type. This forces callers to explicitly deal with situations where re-typing is not possible and it ensures that the function never creates invalid casts. This will make it easier to track down future related bugs because internal errors should originate from the call site of `tree.ReType` rather than from logic further along in the optimization process (in the case of cockroachdb#76807 the internal error originated from the logical props builder when it attempted to lookup the volatility of the invalid cast). This commit also adds special logic to make casts from any tuple type to `types.AnyTuple` valid immutable, implicit casts. Evaluation of these casts are no-ops. Users cannot construct these casts, but they are built by optbuilder in some cases. Fixes cockroachdb#76807 Release justification: This is a low-risk change that fixes a minor bug. Release note (bug fix): A bug has been fixed that caused internal errors when COALESCE and IF expressions had inner expressions with different types that could not be cast to a common type.
mgartner
added a commit
that referenced
this pull request
Mar 23, 2022
The optbuilder no longer creates invalid casts when building COALESCE and IF expressions that have children with different types. Expressions that previously caused internal errors now result in user-facing errors. Both UNION and CASE expressions had similar bugs that were recently fixed in #75219 and #76193. This commit also updates the `tree.ReType` function to return `ok=false` if there is no valid cast to re-type the expression to the given type. This forces callers to explicitly deal with situations where re-typing is not possible and it ensures that the function never creates invalid casts. This will make it easier to track down future related bugs because internal errors should originate from the call site of `tree.ReType` rather than from logic further along in the optimization process (in the case of #76807 the internal error originated from the logical props builder when it attempted to lookup the volatility of the invalid cast). This commit also adds special logic to make casts from any tuple type to `types.AnyTuple` valid immutable, implicit casts. Evaluation of these casts are no-ops. Users cannot construct these casts, but they are built by optbuilder in some cases. Fixes #76807 Release justification: This is a low-risk change that fixes a minor bug. Release note (bug fix): A bug has been fixed that caused internal errors when COALESCE and IF expressions had inner expressions with different types that could not be cast to a common type.
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.
The optbuilder adds casts in order to match non-identical left/right
types of set operations like UNION (see #60560). This change prevents
the optbuilder from adding casts that are invalid, which caused internal
errors. Now, a user error is thrown if no such cast exists from the left
or right input type to the output type.
Fixes #70831
Release note (bug fix): A bug has been fixed that caused internal errors
in queries with set operations, like UNION, when corresponding columns
on either side of the set operation were not the same. This error only
occurred with a limited set of types. This bug is present in versions
20.2.6+, 21.1.0+, and 21.2.0+.