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

SortValues should fail if SecondaryKey coder is not deterministic #20109

Closed
damccorm opened this issue Jun 4, 2022 · 3 comments · Fixed by #23742
Closed

SortValues should fail if SecondaryKey coder is not deterministic #20109

damccorm opened this issue Jun 4, 2022 · 3 comments · Fixed by #23742
Assignees
Labels
bug done & done Issue has been reviewed after it was closed for verification, followups, etc. extensions good first issue java P1 sorter

Comments

@damccorm
Copy link
Contributor

damccorm commented Jun 4, 2022

SortValues transform uses lexicographical sorting on encoded binaries and might not work correctly if the coder is not deterministic.

Imported from Jira BEAM-8985. Original Jira may contain additional context.
Reported by: sinisa_lyh.

@kennknowles
Copy link
Member

@reuvenlax FYI since I know you work with sortvalues a bit

@TheNeuralBit
Copy link
Member

Is this actually a P1 and a good first issue?

@kennknowles
Copy link
Member

Ha. Good point. Independently, both statements are true:

  • It is pretty straightforward to go in to the transform and add the check.
  • It is a data loss risk if the coder is nondeterministic.

@kennknowles kennknowles self-assigned this Oct 19, 2022
@github-actions github-actions bot added this to the 2.44.0 Release milestone Oct 20, 2022
@apilloud apilloud added the done & done Issue has been reviewed after it was closed for verification, followups, etc. label Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug done & done Issue has been reviewed after it was closed for verification, followups, etc. extensions good first issue java P1 sorter
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants