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

distsql: support ARRAY_AGG in distsql #15939

Closed
RaduBerinde opened this issue May 15, 2017 · 15 comments
Closed

distsql: support ARRAY_AGG in distsql #15939

RaduBerinde opened this issue May 15, 2017 · 15 comments
Milestone

Comments

@RaduBerinde
Copy link
Member

We currently don't support arrays in DistSQL streams. The main thing that's missing is a way to encode them; this will have to be implemented to support arrays in table columns (see #15818). Once that's done we should enable support in DistSQL as well.

CC @asubiotto

@RaduBerinde
Copy link
Member Author

Part of this work we should also implement ARRAY_AGG in distsql.

@cuongdo
Copy link
Contributor

cuongdo commented May 16, 2017

Given that we currently only support arrays for virtual schemas, is this currently affecting anything? Or is this a natural part of our work to support ARRAYs?

@RaduBerinde
Copy link
Member Author

We support arrays in select expressions (and there is ARRAY_AGG).

@a6802739
Copy link
Contributor

a6802739 commented Jun 1, 2017

@RaduBerinde could I have a try for this? And If you could give me some reference, I would appreciate it very much.

@RaduBerinde
Copy link
Member Author

Hi @a6802739 - to work on this we need array encoding code to be in place. This is only in the RFC stage at this point (#16172).

@a6802739
Copy link
Contributor

a6802739 commented Jun 2, 2017

@RaduBerinde Thank you, I will wait until it's ready.

@cuongdo
Copy link
Contributor

cuongdo commented Jul 14, 2017

@RaduBerinde can you reassign?

@RaduBerinde RaduBerinde assigned asubiotto and unassigned RaduBerinde Jul 14, 2017
@RaduBerinde
Copy link
Member Author

@asubiotto I'll put this on your plate for now. We may need to move to 1.2 (not sure what is the current state of array encoding for table columns).

@jordanlewis
Copy link
Member

cc @justinj

@rjnn
Copy link
Contributor

rjnn commented Jul 15, 2017

@cuongdo This is not going to make it into 1.1, @asubiotto's other big priority is getting external storage for HashJoiners and there isn't time for both big projects to make it in in 1.1, so lets move it to 1.2 unless we can get more hands on deck between now and the 1.1 feature freeze.

@vivekmenezes vivekmenezes modified the milestones: 1.2, 1.1 Aug 10, 2017
@RaduBerinde RaduBerinde assigned justinj and unassigned asubiotto Sep 6, 2017
@RaduBerinde
Copy link
Member Author

Fixed in #18243.

@RaduBerinde
Copy link
Member Author

Part of this work we should also implement ARRAY_AGG in distsql.

This is still to be done.

@RaduBerinde RaduBerinde reopened this Sep 8, 2017
@RaduBerinde RaduBerinde changed the title distsql: support arrays in streams distsql: support ARRAY_AGG in distsql Oct 9, 2017
@vivekmenezes
Copy link
Contributor

assigning this issue to abhishek. Can't assign at the moment because he's still not a contributor

@abhimadan
Copy link
Contributor

I think that adding ARRAY_AGG support should only involve removing the explicit check for it in the physical planner, and adding it to the AggregatorSpec_Func proto enum. However, when I tried to do this, I ran into the CLI error pq: unsupported result type: anyelement.

After some investigating, I found that the aggregator processor calls tree.Builtin.FixedType(), which in turn calls returnTypeToFixedType(). This function relies on the fact that its ReturnTyper argument returns a valid type when passed an empty expression list. However, ARRAY_AGG does not follow this assumption (see here). We can actually see this bug manifest in the user docs (see here), where ARRAY_AGG returns anyelement instead of an array.

I'm getting a PR ready to fix this issue.

abhimadan added a commit to abhimadan/cockroach that referenced this issue Jan 10, 2018
As referenced in cockroachdb#15939, ARRAY_AGG erroneously returns `anyelement` as
its fixed return type. This follows the convention of existing array
builtin functions to have overloads for all non-array types, which
allows the fixed return type to be known for each overload.

Release note (bug fix): None
abhimadan added a commit to abhimadan/cockroach that referenced this issue Jan 11, 2018
As referenced in cockroachdb#15939, ARRAY_AGG erroneously returns `anyelement` as
its fixed return type. This follows the convention of existing array
builtin functions to have overloads for all non-array types, which
allows the fixed return type to be known for each overload. Whenever
possible, we still use the expression's type so that aliases without
explicit overloads return the expected type.

Release note (bug fix): None
abhimadan added a commit to abhimadan/cockroach that referenced this issue Jan 16, 2018
As referenced in cockroachdb#15939, ARRAY_AGG erroneously returns `anyelement` as
its fixed return type. This follows the convention of existing array
builtin functions to have overloads for all non-array types, which
allows the fixed return type to be known for each overload. Whenever
possible, we still use the expression's type so that aliases without
explicit overloads return the expected type.

Release note (bug fix): None
@abhimadan
Copy link
Contributor

Fixed by #21475.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants