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

Fixes intersection functions (uint64, int64, []byte) #6067

Merged
merged 5 commits into from Jun 1, 2020

Conversation

farazdagi
Copy link
Contributor

@farazdagi farazdagi commented Jun 1, 2020

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

  • Fixes issue with intersection functions, when argument list has more than 2 slices (issue is reported by audit team)

Which issues(s) does this PR fix?

Fixes #6029

Other notes for review

  • This is an alternative PR to the one that has been created by Terence: Fix intersection functions #6050
  • Terence's PR was blocked by what seemed to be issues in init-sync fetcher tests. The issue proved to be not in tests, but in how intersection was implemented: it modified passed in argument (the first slice), which was unexpected. While debugging the issue, I came up with this alternative implementation.
  • This PR uses different implementation (still the one based on map - so efficiency is the same), which doesn't require modification of function argument. Also, I believe that proposed implementation is easier to reason about and more generic (so, should we want to use reflection - the very same code can be used for int64/uint64 and []byte - in previous implementation []byte has somewhat different code).
  • More test cases has been checked
  • Proposed action: either cherry-pick from this PR, or merge this one and close the original Fix intersection functions #6050.

@farazdagi farazdagi self-assigned this Jun 1, 2020
@farazdagi farazdagi marked this pull request as ready for review June 1, 2020 12:00
@farazdagi farazdagi requested a review from a team as a code owner June 1, 2020 12:00
@farazdagi farazdagi requested a review from nisdas June 1, 2020 12:00
@farazdagi farazdagi added the Ready For Review A pull request ready for code review label Jun 1, 2020
Copy link
Member

@terencechain terencechain 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 good to me. Thank you for fixing this.
Happy to close the original one

@terencechain terencechain merged commit 261a343 into master Jun 1, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-intersection-alt branch June 1, 2020 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IntersectionUint64, IntersectionInt64, IntersectionByteSlices return union if more than 2 slices are provided
2 participants