Skip to content
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

sql: support array_agg(record) #70029

Closed
nvanbenschoten opened this issue Sep 10, 2021 · 6 comments · Fixed by #70332
Closed

sql: support array_agg(record) #70029

nvanbenschoten opened this issue Sep 10, 2021 · 6 comments · Fixed by #70332
Assignees
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-typing SQLtype inference, typing rules, type compatibility. A-tools-postgrest C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@nvanbenschoten
Copy link
Member

nvanbenschoten commented Sep 10, 2021

Needed for #69010.

In Postgres:

> SELECT array_agg(ROW(1, 2));
 array_agg
-----------
 {"(1,2)"}

In CockroachDB:

> SELECT array_agg(ROW(1, 2));
ERROR: unknown signature: array_agg(tuple{int, int})

Prototype:

diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go
index 73ac8b8c4c..9ad1baa460 100644
--- a/pkg/sql/sem/builtins/builtins.go
+++ b/pkg/sql/sem/builtins/builtins.go
@@ -7057,6 +7057,8 @@ func arrayBuiltin(impl func(*types.T) tree.Overload) builtinDefinition {
                        overloads = append(overloads, impl(typ))
                }
        }
+       // WIP: this isn't actually the right way to do this.
+       overloads = append(overloads, impl(types.AnyTuple))
        return builtinDefinition{
                props:     tree.FunctionProperties{Category: categoryArray},
                overloads: overloads,

Epic CRDB-10300

@nvanbenschoten nvanbenschoten added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-typing SQLtype inference, typing rules, type compatibility. labels Sep 10, 2021
@blathers-crl blathers-crl bot added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Sep 10, 2021
@nvanbenschoten nvanbenschoten added the A-sql-pgcompat Semantic compatibility with PostgreSQL label Sep 10, 2021
@rafiss
Copy link
Collaborator

rafiss commented Sep 14, 2021

I think we might need to resolve #32715 for this

My latest attempt was in #63996 but I stopped working on it

@jordanlewis
Copy link
Member

You could do it by editing this code in distsql_physical_planner

	case *tree.CastExpr:
		// TODO (rohany): I'm not sure why this CastExpr doesn't have a type
		//  annotation at this stage of processing...
		if typ, ok := tree.GetStaticallyKnownType(t.Type); ok {
			switch typ.Family() {
			case types.OidFamily:
				v.err = newQueryNotSupportedErrorf("cast to %s is not supported by distsql", t.Type)
				return false, expr
			}
		}
	}

@jordanlewis
Copy link
Member

You could also set the array_agg overloads that take tuples to be DistSQLBlocklist=true, right?

@rafiss
Copy link
Collaborator

rafiss commented Sep 16, 2021

I don't think the issue is DistSQL -- the problem is the vectorized engine:

        (XX000) internal error: unexpected error from the vectorized engine: couldn't decode type anyelement
        error.go:88: in func1()
        DETAIL: stack trace:
        github.com/cockroachdb/cockroach/pkg/sql/colexecerror/error.go:88: func1()
        runtime/panic.go:965: gopanic()
        github.com/cockroachdb/cockroach/pkg/sql/colexecerror/error.go:192: InternalError()
        github.com/cockroachdb/cockroach/pkg/sql/colflow/colrpc/inbox.go:307: func1()
        runtime/panic.go:965: gopanic()
        github.com/cockroachdb/cockroach/pkg/sql/colexecerror/error.go:192: InternalError()
        github.com/cockroachdb/cockroach/pkg/sql/colflow/colrpc/inbox.go:379: func2()
        github.com/cockroachdb/cockroach/pkg/sql/colmem/allocator.go:301: PerformOperation()
        github.com/cockroachdb/cockroach/pkg/sql/colflow/colrpc/inbox.go:377: Next()
        github.com/cockroachdb/cockroach/pkg/sql/colexec/invariants_checker.go:85: Next()
        github.com/cockroachdb/cockroach/pkg/sql/colflow/stats.go:99: Next()
        github.com/cockroachdb/cockroach/pkg/sql/colexecop/operator.go:371: Next()

The AnyTuple type contains an anyelement in it which can't be handled here

return nil, buf, errors.Errorf("couldn't decode type %s", t)

@rafiss
Copy link
Collaborator

rafiss commented Sep 16, 2021

Ah ok, nvm, that does also block it from using the vectorized engine. I just wasn't seeing that since I was setting the property in the wrong place.

@yuzefovich
Copy link
Member

On a quick glance, the vectorized engine should work here in the same manner as the row engine (because we're using the same representation - coldata.DatumVec), and the stack trace posted above might have originated because of the usage of the DistSQL. If this feature is blocklisted for DistSQL, the internal error above maybe wouldn't occur.

@craig craig bot closed this as completed in 16ea3e8 Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-typing SQLtype inference, typing rules, type compatibility. A-tools-postgrest C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants