sql: resolve values types after building scalars in optbuilder #103078
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
Previously, we would build scalars in VALUES clauses after resolving the value column types. However, for UDFs, we modify the type if it's a record-returning function during the build. In this change we reverse the order so that we build scalars and then resolve types. This bug was introduced by cockroachdb#98162. Epic: None Fixes: cockroachdb#102718 Release note: This fixes a bug in VALUES clauses containing a call to a record-returning UDF that could manifest as an internal error in some queries.
|
This change is |
rharding6373
added
the
backport-23.1.x
DrewKimball
approved these changes
May 11, 2023
There was a problem hiding this comment.
I think this fix is ok, but why can't the correct type be obtained during type-checking? Should we be doing the record-returning type resolving logic during function creation instead?
Reviewed all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained
rharding6373
commented
May 16, 2023
There was a problem hiding this comment.
TFTR. I'd consider refactoring UDFs to try to resolve record types at function creation in a future PR, I think that's doable. Thanks for the suggestion!
Reviewable status:
|
Actually, let me try to do a refactor this week before submitting this PR (though if it's successful it may be hefty for a backport). |
|
I looked into refactoring this on the train, and it's not a simple refactor. The main thing that would have to change is we would have to save information about the function (in this case the types to which the RECORD resolves). As of my current understanding we'd have to modify both the schema changer code path and exec code path. I think that where and how to save this information in a way that we can get it again when the function is referenced is the challenge. As such, I'm going to move forward with merging this PR in order to fix the bug for now, and maybe look into refactoring in the future. |
|
bors r+ |
|
Build succeeded: |
DrewKimball
added a commit
to DrewKimball/cockroach
that referenced
this pull request
Aug 27, 2024
The type of a RECORD-returning UDF is not fully resolved until after it is fully built within the optbuilder. Therefore, a VALUES expression with a UDF can only determine its column types after building its rows. We accounted for this in cockroachdb#103078, but that fix was incomplete - building the row resolves the type of the UDF, but does not update the type annotation for any expressions that wrap it (for example, a COALESCE). This commit fully fixes the type resolution bug by type checking the row once again after it is built. This ensures that the UDF's resolved type is correctly propagated to parent expressions. Fixes cockroachdb#117101 Release note (bug fix): Fixed a bug that would cause an internal error when the result of a RECORD-returning UDF was wrapped by another expression (such as COALESCE) within a VALUES clause.
DrewKimball
added a commit
to DrewKimball/cockroach
that referenced
this pull request
Dec 17, 2024
The type of a RECORD-returning UDF is not fully resolved until after it is fully built within the optbuilder. Therefore, a VALUES expression with a UDF can only determine its column types after building its rows. We accounted for this in cockroachdb#103078, but that fix was incomplete - building the row resolves the type of the UDF, but does not update the type annotation for any expressions that wrap it (for example, a COALESCE). This commit fully fixes the type resolution bug by type checking the row once again after it is built. This ensures that the UDF's resolved type is correctly propagated to parent expressions. Fixes cockroachdb#117101 Release note (bug fix): Fixed a bug that would cause an internal error when the result of a RECORD-returning UDF was wrapped by another expression (such as COALESCE) within a VALUES clause.
DrewKimball
added a commit
to DrewKimball/cockroach
that referenced
this pull request
Dec 17, 2024
The type of a RECORD-returning UDF is not fully resolved until after it is fully built within the optbuilder. Therefore, a VALUES expression with a UDF can only determine its column types after building its rows. We accounted for this in cockroachdb#103078, but that fix was incomplete - building the row resolves the type of the UDF, but does not update the type annotation for any expressions that wrap it (for example, a COALESCE). This commit fully fixes the type resolution bug by type checking the row once again after it is built. This ensures that the UDF's resolved type is correctly propagated to parent expressions. Fixes cockroachdb#117101 Release note (bug fix): Fixed a bug that would cause an internal error when the result of a RECORD-returning UDF was wrapped by another expression (such as COALESCE) within a VALUES clause.
DrewKimball
added a commit
to DrewKimball/cockroach
that referenced
this pull request
Dec 17, 2024
The type of a RECORD-returning UDF is not fully resolved until after it is fully built within the optbuilder. Therefore, a VALUES expression with a UDF can only determine its column types after building its rows. We accounted for this in cockroachdb#103078, but that fix was incomplete - building the row resolves the type of the UDF, but does not update the type annotation for any expressions that wrap it (for example, a COALESCE). This commit fully fixes the type resolution bug by type checking the row once again after it is built. This ensures that the UDF's resolved type is correctly propagated to parent expressions. Fixes cockroachdb#117101 Release note (bug fix): Fixed a bug that would cause an internal error when the result of a RECORD-returning UDF was wrapped by another expression (such as COALESCE) within a VALUES clause.
DrewKimball
added a commit
to DrewKimball/cockroach
that referenced
this pull request
Dec 18, 2024
The type of a RECORD-returning UDF is not fully resolved until after it is fully built within the optbuilder. Therefore, a VALUES expression with a UDF can only determine its column types after building its rows. We accounted for this in cockroachdb#103078, but that fix was incomplete - building the row resolves the type of the UDF, but does not update the type annotation for any expressions that wrap it (for example, a COALESCE). This commit fully fixes the type resolution bug by type checking the row once again after it is built. This ensures that the UDF's resolved type is correctly propagated to parent expressions. Fixes cockroachdb#117101 Release note (bug fix): Fixed a bug that would cause an internal error when the result of a RECORD-returning UDF was wrapped by another expression (such as COALESCE) within a VALUES clause.
craig bot
pushed a commit
that referenced
this pull request
Dec 18, 2024
129706: opt: type check VALUES rows after building them r=DrewKimball a=DrewKimball The type of a RECORD-returning UDF is not fully resolved until after it is fully built within the optbuilder. Therefore, a VALUES expression with a UDF can only determine its column types after building its rows. We accounted for this in #103078, but that fix was incomplete - building the row resolves the type of the UDF, but does not update the type annotation for any expressions that wrap it (for example, a COALESCE). This commit fully fixes the type resolution bug by type checking the row once again after it is built. This ensures that the UDF's resolved type is correctly propagated to parent expressions. Fixes #117101 Release note (bug fix): Fixed a bug that would cause an internal error when the result of a RECORD-returning UDF was wrapped by another expression (such as COALESCE) within a VALUES clause. 137681: sql: Internal sessions exempt from disallow_full_table_scans setting r=spilchen a=spilchen Previously, enabling the cluster setting sql.defaults.disallow_full_table_scans.enabled prevented the creation of indexes due to the internal table scans required during the process. This change ensures that the setting is bypassed for internal operations, while still applying to user queries. Epic: None Closes: #137404 Release note (bug fix): Internal scans are now exempt from the sql.defaults.disallow_full_table_scans.enabled setting, allowing index creation even when the cluster setting is active. Co-authored-by: Drew Kimball <drewk@cockroachlabs.com> Co-authored-by: Matt Spilchen <matt.spilchen@cockroachlabs.com>
blathers-crl bot
pushed a commit
that referenced
this pull request
Dec 18, 2024
The type of a RECORD-returning UDF is not fully resolved until after it is fully built within the optbuilder. Therefore, a VALUES expression with a UDF can only determine its column types after building its rows. We accounted for this in #103078, but that fix was incomplete - building the row resolves the type of the UDF, but does not update the type annotation for any expressions that wrap it (for example, a COALESCE). This commit fully fixes the type resolution bug by type checking the row once again after it is built. This ensures that the UDF's resolved type is correctly propagated to parent expressions. Fixes #117101 Release note (bug fix): Fixed a bug that would cause an internal error when the result of a RECORD-returning UDF was wrapped by another expression (such as COALESCE) within a VALUES clause.
blathers-crl bot
pushed a commit
that referenced
this pull request
Dec 18, 2024
The type of a RECORD-returning UDF is not fully resolved until after it is fully built within the optbuilder. Therefore, a VALUES expression with a UDF can only determine its column types after building its rows. We accounted for this in #103078, but that fix was incomplete - building the row resolves the type of the UDF, but does not update the type annotation for any expressions that wrap it (for example, a COALESCE). This commit fully fixes the type resolution bug by type checking the row once again after it is built. This ensures that the UDF's resolved type is correctly propagated to parent expressions. Fixes #117101 Release note (bug fix): Fixed a bug that would cause an internal error when the result of a RECORD-returning UDF was wrapped by another expression (such as COALESCE) within a VALUES clause.
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.
Previously, we would build scalars in VALUES clauses after resolving the value column types. However, for UDFs, we modify the type if it's a record-returning function during the build. In this change we reverse the order so that we build scalars and then resolve types.
This bug was introduced by #98162.
Epic: None
Fixes: #102718
Release note: This fixes a bug in VALUES clauses containing a call to a record-returning UDF that could manifest as an internal error in some queries.