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

feat(expr): support array_distinct #8315

Merged
merged 15 commits into from Mar 7, 2023

Conversation

snipekill
Copy link
Contributor

@snipekill snipekill commented Mar 3, 2023

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Feature #7765

Checklist For Contributors

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

Types of user-facing changes

  • SQL commands, functions, and operators

Release note

Support array_distinct.

@lmatz
Copy link
Contributor

lmatz commented Mar 3, 2023

Thank you for your contribution!
Could you add some e2e tests and/or unit tests for array_distinct https://github.com/risingwavelabs/risingwave/tree/main/e2e_test/batch/functions

@snipekill
Copy link
Contributor Author

Thank you for your contribution! Could you add some e2e tests and/or unit tests for array_distinct https://github.com/risingwavelabs/risingwave/tree/main/e2e_test/batch/functions

Yeah sure

@snipekill
Copy link
Contributor Author

@lmatz To get distinct values, I am using Hashset, as it is unordered, writing the output for e2e tests can be tricky, as output order of elements in the array can change. Do, I sort the input and use dedup instead, it would not be that performant, and I can sort the output after.

@xiangjinwu
Copy link
Contributor

xiangjinwu commented Mar 3, 2023

@lmatz To get distinct values, I am using Hashset, as it is unordered, writing the output for e2e tests can be tricky, as output order of elements in the array can change. Do, I sort the input and use dedup instead, it would not be that performant, and I can sort the output after.

There is a way to get deterministic output with HashSet rather than sorting: scan the array and skip those already in HashSet, add new values to output and HashSet.

https://docs.rs/itertools/0.10.5/itertools/trait.Itertools.html#method.unique

@snipekill
Copy link
Contributor Author

@lmatz To get distinct values, I am using Hashset, as it is unordered, writing the output for e2e tests can be tricky, as output order of elements in the array can change. Do, I sort the input and use dedup instead, it would not be that performant, and I can sort the output after.

There is a way to get deterministic output with HashSet rather than sorting: scan the array and skip those already in HashSet, add new values to output and HashSet.

https://docs.rs/itertools/0.10.5/itertools/trait.Itertools.html#method.unique

Makes sense will change the logic

@snipekill
Copy link
Contributor Author

@fuyufjh @lmatz have made all the required changes, added unit and e2e tests as well, can you check the code pertaining to type checking of inputs to match function signature.

Copy link
Contributor

@xiangjinwu xiangjinwu left a comment

Choose a reason for hiding this comment

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

Rest lgtm. Thank you!

src/expr/src/expr/expr_array_distinct.rs Outdated Show resolved Hide resolved
src/frontend/src/expr/type_inference/func.rs Outdated Show resolved Hide resolved
@xiangjinwu xiangjinwu added this pull request to the merge queue Mar 7, 2023
Merged via the queue into risingwavelabs:main with commit 0a6638c Mar 7, 2023
9 checks passed
@xiangjinwu xiangjinwu added the user-facing-changes Contains changes that are visible to users label Mar 7, 2023
@fuyufjh
Copy link
Contributor

fuyufjh commented Mar 7, 2023

Good job! Thanks a lot for your contribution! @snipekill

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user-facing-changes Contains changes that are visible to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants