sql: reject nested array constructors - #174733
Open
Alignyx wants to merge 1 commit into
Open
Conversation
The array-element support check rejects several unsupported scalar types but allows arrays. Consequently ARRAY[...] and ARRAY(subquery) can build nested arrays even though their encoding is unsupported. Casting these values to text quotes inner arrays as strings and produces wrong results. Reject array-valued elements through the shared support check, using the existing multidimensional-array feature issue. Keep internal array types, arrays of tuples containing arrays, and the separately registered array_agg(array) overload unchanged. Add constructor and type-support regressions. Update existing unsupported constructor expectations and preserve unrelated optimizer-rule coverage using supported array and tuple expressions. Resolves: cockroachdb#146717 Epic: None Release note (bug fix): Nested ARRAY[...] and ARRAY(subquery) constructors now consistently return an unsupported-feature error instead of producing incorrectly formatted multidimensional arrays.
|
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This file contains hidden or 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
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.
Summary
Reject array-valued elements in
ARRAY[...]andARRAY(subquery)through the existing shared array-element support check. This addresses #146717 by reporting the existing unsupported multidimensional-array feature instead of producing an incorrectly formatted nested result.This is a constructor/type-validation fix, not an implementation of multidimensional arrays or a universal rejection of every nested-array-producing operation. In particular, the existing
array_agg(array)exception is deliberately unchanged. Guidance on that remaining scope is requested below.Problem and root cause
The reported expression is:
It succeeds on the baseline but produces
{"{a}"}. This quotes the inner array's text as an outer element rather than representing the intended multidimensional value{{a}}. The issue explicitly accepts an unsupported-feature error because multidimensional arrays are not fully supported.Both ARRAY-constructor paths in the optimizer builder already call
types.CheckArrayElementType. That function delegates toIsValidArrayElementType, which rejects unsupported TSQUERY, TSVECTOR and PGVECTOR elements but has noArrayFamilycase. Its default branch therefore accepts an array as an array element. An unsupported nested value can be built and constant-folded before formatting exposes the problem; other execution/encoding paths can reject it later instead.The missing check is at the feature-support boundary, not simply in the text formatter. Changing the formatter alone would not supply the missing nested-array execution, encoding, or storage support.
How the discrepancy was localized
The investigation compared alternative representations of the same intended multidimensional value:
Three isolated baseline matrices consistently observed constructor success with
{"{a}"}, while the dimensional literal and explicit dimensional annotation rejected the unsupported feature with SQLSTATE0A000and a reference to #32552. Row-mode and forced-DistSQL controls did not make the original text result correct.This is an error-based metamorphic relation: consistency of the support/rejection boundary across equivalent intended constructions, rather than equality between two successful CockroachDB result sets. The error controls do not have executable plans. For the successful baseline constructor,
EXPLAIN (OPT, VERBOSE)already contained the incorrectly formatted constant in a VALUES plan.EXPLAIN ANALYZEconfirmed execution of a one-row VALUES operator; the separate SELECT returned the incorrect text. The comparison therefore localizes the discrepancy to planning/type-support validation; it is not a claim of two successful runtime branch traces.On the candidate, the constructors also reject during planning. The literal and type-annotation controls retain their existing errors. Consistency here means the unsupported-feature class/SQLSTATE and feature reference, not identical complete error wording across syntactic forms.
Repair method and scope
The only production change is the missing case in
pkg/sql/types/types.go:CheckArrayElementTypeconverts this into the existing unimplemented-feature error. Reusing the shared predicate covers its existing constructor, scalar-evaluation and type-validation callers without adding ad hoc checks to each syntax form. The constructor rejection is based on element type, including typed NULL/empty inputs, rather than depending on whether an unsupported value happens to be produced at runtime.The patch does not change
types.MakeArray, type serialization,upgradeType,UserDefined, arithmetic, or text formatting. Normal flat arrays, scalar strings containing braces, and arrays of tuples whose fields contain arrays remain supported. Existing TSQUERY/TSVECTOR/PGVECTOR restrictions remain unchanged.The other eight changed files are tests or fixtures:
Relation to the previously closed PR
#168470 targeted #167545, not #146717. It proposed relaxing nested-array unmarshaling for user-defined types; #146717 was cited during review as a risk of admitting incompletely supported arrays-of-arrays. That PR was closed in favor of rejecting domain-of-array creation. The replacement #168715 was merged.
This patch follows the early-rejection direction for a different entry point. The baseline already contains the domain-of-array/tuple creation restrictions, and they remain unchanged. The nested-array unmarshaling assertion is not relaxed, and no UDT exception is introduced. This history supports the distinction in approach; it is not a claim of prior approval of this particular patch.
Completed local verification
The candidate is
49af5ea57c9943cdba56cff708f4fad3241d2bda, based independently on8812064a015d2faf99d3fc7e15880f94042954b0.array,aggregate, andsrfslogic files pass in local, local-vec-off, fakedist, fakedist-vec-off and fakedist-disk. The new regression has RUN/PASS records in all five configurations.pg_catalogpasses its selected local-only configuration.These describe completed local validation of the existing candidate, not upstream CI status. No new test execution is implied by opening this PR.
Known limitations and compatibility implications
array_agg(array)remains an exception. Its existing explicitly registered array-input overload can still produce nested output such as{"{b,c}"}. Overload generation first checks a scalar element type, then synthesizes the array-input overload without reapplying this predicate to that array type. It is explicitly blocked from distributed evaluation because nested-array value encoding is unsupported. This PR does not remove that overload or correct its text representation; an existing regression and the candidate's additional checks preserve that behavior.::TEXT. Applications relying on that incomplete local-only behavior will receive an unsupported-feature error. Ordinary supported one-dimensional arrays are not intended to change.MakeArraynesting available does not make nested values generally supported SQL values.The deliberate preservation of
array_agg(array)means this should be reviewed as a bounded constructor fix, not as a claim that all concerns about nested arrays have been eliminated.Questions for review and possible follow-up
Would you like me to continue addressing the remaining nested-array entry points, especially
array_agg(array), in this PR or in a separate follow-up? Is preserving that existing overload the preferred compatibility boundary for this constructor fix, or should it also become an unsupported-feature error?If further rejection work is desired, the next step would be to audit array-input overload registration and other array-producing paths, distinguish nesting operations from flattening operations such as
array_cat_agg, and assess the compatibility impact before changing the accepted signatures or results. That follow-up is not implemented or validated here. Full multidimensional support would instead need a broader design across type representation, formatting, execution and encoding; it should not be implied by a formatter-only change or an unmarshaling exception.Should remaining work be tracked under #146717, under the broader #32552, or in a separate issue? The existing commit has an issue-closing trailer, so the intended tracking boundary should be agreed before merge.
See also: #146717, #32552
Epic: none
Release note (bug fix): Nested ARRAY[...] and ARRAY(subquery) constructors now consistently return an unsupported-feature error instead of producing incorrectly formatted multidimensional arrays.