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 uniqThetaSketch #22609

Merged
merged 18 commits into from
Apr 19, 2021
Merged

add uniqThetaSketch #22609

merged 18 commits into from
Apr 19, 2021

Conversation

pingyu
Copy link
Contributor

@pingyu pingyu commented Apr 4, 2021

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

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

  • Add uniqThetaSketch to support Theta Sketch in ClickHouse.

Detailed description / Documentation draft:

  • Added document at docs/en/sql-reference/aggregate-functions/reference/uniqthetasketch.md

@robot-clickhouse robot-clickhouse added doc-alert pr-feature Pull request with new product feature labels Apr 4, 2021
@robot-clickhouse robot-clickhouse added the submodule changed At least one submodule changed in this PR. label Apr 4, 2021
@Avogar Avogar self-assigned this Apr 6, 2021
@pingyu
Copy link
Contributor Author

pingyu commented Apr 9, 2021

The failure of build seems to be caused by DataSketches submodule is not checked out.
How can I fix it ? @Avogar

@Avogar
Copy link
Member

Avogar commented Apr 9, 2021

We should add the needed submodule in this list:

SUBMODULES_TO_UPDATE=(

I will do it.

@Avogar
Copy link
Member

Avogar commented Apr 9, 2021

You also should support case when ENABLE_DATASKETCHES=0 in code. Now it just cannot build with it. Put all logic with submodule under if USE_DATASKETCHES and throw an exception when it's not enabled and uniqThetaSketch is called.
I enabled it by default in fasttest to see other tests. Maybe we should disable it in the future, I will think about it.

@pingyu
Copy link
Contributor Author

pingyu commented Apr 10, 2021

Comments addressed. PTAL, thanks~ @Avogar

@Avogar
Copy link
Member

Avogar commented Apr 13, 2021

LGTM, some problems with synchronization again, I will fix it and will merge.

@Avogar
Copy link
Member

Avogar commented Apr 13, 2021

I put all tests with uniqThetaSketch in one file to have an ability to disable it when we cannot use datasketches submodule. Lets wait for the tests and if there are no errors - merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature Pull request with new product feature submodule changed At least one submodule changed in this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants