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: Add overloads to ARRAY_AGG #21377

Merged
merged 1 commit into from
Jan 16, 2018

Conversation

abhimadan
Copy link
Contributor

As referenced in #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 abhimadan requested a review from a team January 10, 2018 18:06
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@CLAassistant
Copy link

CLAassistant commented Jan 10, 2018

CLA assistant check
All committers have signed the CLA.

@abhimadan abhimadan requested a review from justinj January 10, 2018 20:26
@abhimadan abhimadan force-pushed the array-agg-overloads branch from e77a27f to 68a5d94 Compare January 10, 2018 20:28
@justinj
Copy link
Contributor

justinj commented Jan 10, 2018

:lgtm: that was much simpler than I expected!


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


docs/generated/sql/aggregates.md, line 4 at r1 (raw file):

<thead><tr><th>Function &rarr; Returns</th><th>Description</th></tr></thead>
<tbody>
<tr><td><code>array_agg(arg1: <a href="bool.html">bool</a>) &rarr; <a href="bool.html">bool</a>[]</code></td><td><span class="funcdesc"><p>Aggregates the selected values into an array.</p>

😢 this is so unfortunate @knz do you know offhand if we have any way of supporting very basic generic functions (not full HM but something that would handle the array situation)?


Comments from Reviewable

@knz
Copy link
Contributor

knz commented Jan 10, 2018

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


docs/generated/sql/aggregates.md, line 4 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

😢 this is so unfortunate @knz do you know offhand if we have any way of supporting very basic generic functions (not full HM but something that would handle the array situation)?

You mean to generate the documentation? Yes we could enhance the doc generator.

If you mean avoid instantiating the overload explicitly, yes I suppose we can bake something ad-hoc but I am not happy at the thought. If we are to invest in typing we should figure out something principled first.


Comments from Reviewable

@abhimadan
Copy link
Contributor Author

abhimadan commented Jan 11, 2018

So when I tried to fix the failing tests, I encountered an ORM compatibility issue with sequelize. It seems as though it expects to receive a string and convert it to an array of strings, but after my change, it receives an array of strings and fails when it tries to call strings functions on it. I'm investigating this more closely now, but as a result of this, I can't merge this PR right now.

@abhimadan abhimadan force-pushed the array-agg-overloads branch 2 times, most recently from 0a1ba01 to dbb5d07 Compare January 11, 2018 19:37
@abhimadan
Copy link
Contributor Author

I managed to work around the ORM compatibility issue by retaining most of the old ReturnTyper logic so that array_agg still retains aliased type information in the return type. @justinj PTAL!

Copy link
Contributor

@justinj justinj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but maybe add some extra tests for the aliased types, like NAME to make sure they still work (and will continue to work).

}
// Whenever possible, use the expression's type, so we can properly
// handle aliased types that don't explicitly have overloads.
return types.TArray{Typ: args[0].ResolvedType()}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A concern I have with this now is that it effectively creates undocumented overloads for array_agg for types that are aliased to types with defined overloads. It might not be a big issue since this function's behaviour is fairly straightforward, but I wanted to at least mention it.

@abhimadan abhimadan force-pushed the array-agg-overloads branch from dbb5d07 to e592b68 Compare January 16, 2018 15:25
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 abhimadan force-pushed the array-agg-overloads branch from e592b68 to 41f88bb Compare January 16, 2018 15:37
@abhimadan abhimadan merged commit efb4957 into cockroachdb:master Jan 16, 2018
@abhimadan abhimadan deleted the array-agg-overloads branch January 16, 2018 15:58
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

Successfully merging this pull request may close these issues.

5 participants