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

json: allow ArrayBuilder to be reused #32716

Merged
merged 1 commit into from Nov 29, 2018

Conversation

Projects
None yet
3 participants
@justinj
Copy link
Member

justinj commented Nov 29, 2018

Fixes #32702.

json_agg previously assumed that once its value was requested, there
would be no more additions to the aggregation. Similarly,
json.ArrayBuilder would panic if values were added to an array after its
construction. This is actually not true in the case of window functions,
which might incrementally add to an aggregation as it gets output.

This commit changes json.ArrayBuilder to no longer panic in this case
and instead allow more values to be added.

However, it thus also modifies the contract of ArrayBuilder to state
that the called may not mutate the array themselves.

Release note (bug fix): fix a panic involving json_agg and window
functions.

json: allow ArrayBuilder to be reused
Fixes #32702.

json_agg previously assumed that once its value was requested, there
would be no more additions to the aggregation. Similarly,
json.ArrayBuilder would panic if values were added to an array after its
construction. This is actually not true in the case of window functions,
which might incrementally add to an aggregation as it gets output.

This commit changes json.ArrayBuilder to no longer panic in this case
and instead allow more values to be added.

However, it thus also modifies the contract of ArrayBuilder to state
that the called may not mutate the array themselves.

Release note (bug fix): fix a panic involving json_agg and window
functions.

@justinj justinj requested a review from jordanlewis Nov 29, 2018

@justinj justinj requested a review from cockroachdb/sql-rest-prs as a code owner Nov 29, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Nov 29, 2018

This change is Reviewable

@jordanlewis
Copy link
Member

jordanlewis left a comment

LGTM.

@justinj

This comment has been minimized.

Copy link
Member

justinj commented Nov 29, 2018

TFTR

bors r+

craig bot pushed a commit that referenced this pull request Nov 29, 2018

Merge #32716
32716: json: allow ArrayBuilder to be reused r=justinj a=justinj

Fixes #32702.

json_agg previously assumed that once its value was requested, there
would be no more additions to the aggregation. Similarly,
json.ArrayBuilder would panic if values were added to an array after its
construction. This is actually not true in the case of window functions,
which might incrementally add to an aggregation as it gets output.

This commit changes json.ArrayBuilder to no longer panic in this case
and instead allow more values to be added.

However, it thus also modifies the contract of ArrayBuilder to state
that the called may not mutate the array themselves.

Release note (bug fix): fix a panic involving json_agg and window
functions.

Co-authored-by: Justin Jaffray <justin@cockroachlabs.com>
@craig

This comment has been minimized.

Copy link

craig bot commented Nov 29, 2018

Build succeeded

@craig craig bot merged commit 4200aea into cockroachdb:master Nov 29, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment