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

Make index or partition filter valid in tuple like (k1, k2)=(v1,v2) #29281

Merged
merged 7 commits into from
Oct 7, 2021

Conversation

lingtaolf
Copy link
Contributor

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

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Primary key index and partition filter can work in tuple

This PR solved problem in #29269

@robot-clickhouse robot-clickhouse added the pr-improvement Pull request with some product improvements label Sep 23, 2021
@vdimir
Copy link
Member

vdimir commented Sep 23, 2021

You have a great example in your issue, let's try to add it as test.

Also there's some commits Merge pull request #1 from ClickHouse/master in this PR, do you mind to rebase it and have just one or two commits with just your work?

@vdimir vdimir self-assigned this Sep 23, 2021
@lingtaolf
Copy link
Contributor Author

lingtaolf commented Sep 23, 2021

You have a great example in your issue, let's try to add it as test.

Also there's some commits Merge pull request #1 from ClickHouse/master in this PR, do you mind to rebase it and have just one or two commits with just your work?

@vdimir Thanks for your reviewing ! I will try to add some tests and rebase the unnecessary commits.

@lingtaolf lingtaolf closed this Sep 23, 2021
@lingtaolf lingtaolf reopened this Sep 23, 2021
@lingtaolf lingtaolf force-pushed the bugfix/tupleFilter branch 4 times, most recently from 97c991f to 9acf05c Compare September 24, 2021 20:12
@den-crane
Copy link
Contributor

den-crane commented Sep 25, 2021

@lingtaolf I guess you can / should use "force..." for tests:

set force_index_by_date=1, force_primary_key=1 ;

--force_index_by_date arg Throw an exception if there is a partition key in a table, and it is not used.
--force_primary_key arg Throw an exception if there is primary key in a table, and it is not used.

@lingtaolf
Copy link
Contributor Author

lingtaolf commented Sep 26, 2021

@lingtaolf I guess you can / should use "force..." for tests:

set force_index_by_date=1, force_primary_key=1 ;

--force_index_by_date arg Throw an exception if there is a partition key in a table, and it is not used.
--force_primary_key arg Throw an exception if there is primary key in a table, and it is not used.

@den-crane Done ! Thanks for you advices .

@CLAassistant
Copy link

CLAassistant commented Sep 28, 2021

CLA assistant check
All committers have signed the CLA.

@vdimir
Copy link
Member

vdimir commented Sep 28, 2021

I fixed fuzzer error and added support both type of expressions (id, value) = (1, 'A') and (1, 'A') = (id, value)

Analyzing of expression like (1, value) = (id, 'A') not implemented yet, we can add it this PR or in other one.

Copy link
Member

@vdimir vdimir left a comment

Choose a reason for hiding this comment

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

Thank you for your changes!

Let's wait for CI and merge if it'll be ok.

@lingtaolf
Copy link
Contributor Author

Thank you for your changes!

Let's wait for CI and merge if it'll be ok.

@vdimir Thanks for your help !

@vdimir
Copy link
Member

vdimir commented Sep 30, 2021

CI stuck for some reason, I'm going to rebase branch

@vdimir vdimir force-pushed the bugfix/tupleFilter branch from ce844e4 to 7444d64 Compare October 1, 2021 14:01
@vdimir
Copy link
Member

vdimir commented Oct 1, 2021

Added a couple of testcases with tuple in tuple and rebase on master, hope final change, will wait CI and merge.

@vdimir
Copy link
Member

vdimir commented Oct 7, 2021

AST fuzzer (UBSan) — ../contrib/libcxx/include/vector:677:16: runtime error: reference binding to

Known solved issue
#29687

@vdimir vdimir merged commit 0422939 into ClickHouse:master Oct 7, 2021
azat added a commit to azat/ClickHouse that referenced this pull request Oct 10, 2021
Fixes: ClickHouse#29281
Fixes: test_cluster_copier/test_three_nodes.py::test
@azat
Copy link
Collaborator

azat commented Oct 10, 2021

Integration tests (asan) fail: 2, passed: 1559, flaky: 21
Integration tests (release) fail: 1, passed: 1574, flaky: 8
Integration tests (thread) fail: 4, passed: 1566, flaky: 14

@vdimir @lingtaolf test_cluster_copier/test_three_nodes.py::test failed for a reason, should be fixed in #29956

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-improvement Pull request with some product improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants