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

Add arraySymmetricDifference function #62262

Closed
wants to merge 60 commits into from
Closed

Conversation

pheepa
Copy link

@pheepa pheepa commented Apr 3, 2024

Exported logic from arrayIntersect to arrayLogicalFunction class that covers handling both intersection and symmetric difference. Added arraySymmetricDifference function with docs and tests.

Changelog category (leave one):

  • New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Added function arraySymmetricDifference. This closes: #61673

Documentation entry for user-facing changes

  • Documentation is written

@CLAassistant
Copy link

CLAassistant commented Apr 3, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ pheepa
✅ rschu1ze
❌ f.abapolov


f.abapolov seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@pheepa
Copy link
Author

pheepa commented Apr 3, 2024

Can someone approve my CLA sign?...
Some commits were made from work pc without linked email in git config. Here said that old commits can't be linked to email, only new ones.

@robot-ch-test-poll robot-ch-test-poll added the pr-feature Pull request with new product feature label Apr 3, 2024
@robot-ch-test-poll
Copy link
Contributor

robot-ch-test-poll commented Apr 3, 2024

This is an automated comment for commit cdc42b5 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check nameDescriptionStatus
A SyncThere's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS⏳ pending
CI runningA meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR⏳ pending
ClickHouse build checkBuilds ClickHouse in various configurations for use in further steps. You have to fix the builds that fail. Build logs often has enough information to fix the error, but you might have to reproduce the failure locally. The cmake options can be found in the build log, grepping for cmake. Use these options and follow the general build process⏳ pending
Fast testNormally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here❌ failure
Mergeable CheckChecks if all other necessary checks are successful❌ failure
Successful checks
Check nameDescriptionStatus
PR CheckThere's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS✅ success
Style checkRuns a set of checks to keep the code style clean. If some of tests failed, see the related log from the report✅ success

@nikitamikhaylov nikitamikhaylov added the can be tested Allows running workflows for external contributors label Apr 3, 2024
@nikitamikhaylov
Copy link
Member

The extracted part should be also useful for #61553

@rschu1ze rschu1ze self-assigned this Apr 4, 2024
utils/check-style/aspell-ignore/en/aspell-dict.txt Outdated Show resolved Hide resolved
src/Functions/array/arraySymmetricDifference.cpp Outdated Show resolved Hide resolved
docs/en/sql-reference/functions/array-functions.md Outdated Show resolved Hide resolved
docs/en/sql-reference/functions/array-functions.md Outdated Show resolved Hide resolved
src/Functions/array/arrayLogicalFunction.cpp Outdated Show resolved Hide resolved
src/Functions/array/arrayLogicalFunction.cpp Outdated Show resolved Hide resolved
src/Functions/array/arrayLogicalFunction.cpp Outdated Show resolved Hide resolved
src/Functions/array/arrayLogicalFunction.cpp Outdated Show resolved Hide resolved
@@ -0,0 +1,548 @@
#include <Columns/ColumnArray.h>
Copy link
Member

Choose a reason for hiding this comment

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

Comment related to this file in general. Please add in strategic places high-level comments what the code/algorithms is doing. For a casual reader, it is almost impossible to follow without tedious debugging to understand what is happening in this file.

(as an example, you could look at src/Functions/array/arrayFold.cpp which I commented recently after a severe bug was found in the initial implementation)

@pheepa

This comment was marked as outdated.

@pheepa

This comment was marked as outdated.

@rschu1ze
Copy link
Member

Can someone approve my CLA sign?...
Some commits were made from work pc without linked email in git config. Here said that old commits can't be linked to email, only new ones.

@pheepa GitHub won't let me do that, but unfortunately we need to have the CLA signed. Also, the commit history of this PR is really messy. What about if you squash all your and my commits into a new commit - make sure that Git uses your verified mail address - and force-push (which is generally bad practice but okay in this case)?

@rschu1ze
Copy link
Member

The test failures are related, could you please check them?

@pheepa
Copy link
Author

pheepa commented Apr 25, 2024

Also, the commit history of this PR is really messy.

I completely agree, lets drop this one, I will reopen new with fixes and add your commits

The test failures are related, could you please check them?

I am trying to fix them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can be tested Allows running workflows for external contributors pr-feature Pull request with new product feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement arraySymmetricDifference function
5 participants