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

rowenc: add value encoding for arrays of tuples #63996

Merged
merged 1 commit into from
Sep 20, 2021

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Apr 21, 2021

fixes #32715
resurrecting an old PR #32871

Previously, queries that required an array of tuples to be encoded would
fail. This could only happen if a distsql query required such a datum to
be sent over the wire. This commit adds an encoding so such queries can
succeed.

A good followup task to do after this is #63995

Release note (bug fix): queries involving arrays of tuples will no
longer spuriously fail due to an encoding error.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rafiss rafiss added the do-not-merge bors won't merge a PR with this label. label Apr 23, 2021
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

This looks nice, but probably needs a decode function too?

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

@rafiss rafiss force-pushed the array-tuples branch 2 times, most recently from 289714c to f27e031 Compare September 16, 2021 03:36
Previously, queries that required an array of tuples to be encoded would
fail. This could only happen if a distsql query required such a datum to
be sent over the wire. This commit adds an encoding so such queries can
succeed.

Release note (bug fix): queries involving arrays of tuples will no
longer spuriously fail due to an encoding error.
@rafiss rafiss changed the title rowenc: add value encoding for arrays of tuples rowenc: add value encoding for arrays of tuples, and support array builtins Sep 16, 2021
@rafiss rafiss requested review from nvanbenschoten, jordanlewis and a team September 16, 2021 16:31
@rafiss rafiss removed the do-not-merge bors won't merge a PR with this label. label Sep 16, 2021
@rafiss rafiss changed the title rowenc: add value encoding for arrays of tuples, and support array builtins rowenc: add value encoding for arrays of tuples Sep 16, 2021
@rafiss rafiss removed the request for review from nvanbenschoten September 16, 2021 19:51
@rafiss rafiss requested review from otan, yuzefovich and a team and removed request for otan September 17, 2021 05:00
Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

:lgtm:

re: the need for a decode function - looks like it is already in place.

Reviewed 5 of 5 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)

@rafiss
Copy link
Collaborator Author

rafiss commented Sep 20, 2021

tftr!

bors r=yuzefovich

@craig
Copy link
Contributor

craig bot commented Sep 20, 2021

This PR was included in a batch that was canceled, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Sep 20, 2021

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Sep 20, 2021

Build succeeded:

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.

distsql: array of tuples cannot be sent over the wire
4 participants