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

Refs #26327 -- Renamed JsonAgg to JSONBAgg. #7554

Closed
wants to merge 1 commit into from

Conversation

atombrella
Copy link
Contributor

@atombrella atombrella commented Nov 14, 2016

Thanks to Christian von Roques for the report.

I added a new flag to indicate availability of JSONB_AGG and fallback to json_agg on PostgreSQL. I tried to test locally for things highlighted in https://www.postgresql.org/docs/9.4/static/datatype-json.html#JSON-KEYS-ELEMENTS but get the same result on both 9.5 and 9.4 using FloatField with the value 1.230e-5.

Are more tests needed? Alternatively, it may be better to disallow use of this function on PostgreSQL 9.4?

@timgraham
Copy link
Member

Please update the ticket as described in the "According to the ticket's flags," section to get feedback on the PR.

@charettes
Copy link
Member

I find it a bit odd that we ship an aggregate for a datatype we don't use for any fields (JsonField usesjsonb).

As I mentioned on #26327 I'd be more in favor to rename the aggregate JsonbAgg instead.

@atombrella atombrella force-pushed the refs_26327 branch 4 times, most recently from 8dc6091 to 9cdb1bf Compare November 17, 2016 20:03
@atombrella
Copy link
Contributor Author

I chose the name JSONBAgg and added a few notes in the documentation about availability. I'm also in favor of choosing a name with b in it to specify that this is only for jsonb.

Copy link
Member

@timgraham timgraham left a comment

Choose a reason for hiding this comment

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

Simon, is this fine with you?


.. versionadded:: 1.11

Returns the input values as a ``JSON`` array.
Returns the input values as a ``JSON`` array. PostgreSQL ships with two
Copy link
Member

Choose a reason for hiding this comment

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

Do you think a single sentence would work here: "Returns ... using JSONB_AGG() (requires PostgreSQL 9.5+)."
I'm not sure if the other sentences are adding much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Perhaps "PostgreSQL ships ..." is unnecessary because it's already in the documentation for JSONField.

…ailability in documentation.

Thanks to Christian von Roques for the report.
@charettes
Copy link
Member

@timgraham to answer your previous question I also think JSONBAgg is the most appropriate name.

@timgraham timgraham changed the title Refs #26327 -- Support for JsonAgg on PostgreSQL 9.4. Refs #26327 -- Renamed JsonAgg to JSONBAgg. Nov 28, 2016
@timgraham
Copy link
Member

merged in aa2cb4c, thanks!

@timgraham timgraham closed this Nov 28, 2016
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.

3 participants