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.

Sign up for GitHub

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

Jump to bottom

Check simple dictionary key is native unsigned integer #48335

Merged
merged 6 commits into from
Apr 12, 2023
Merged

Check simple dictionary key is native unsigned integer #48335

merged 6 commits into from
Apr 12, 2023

Conversation

lzydmxy
Copy link
Contributor

@lzydmxy lzydmxy commented Apr 3, 2023 •
edited

Changelog category (leave one):

  • Improvement

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

Check primary key type for simple dictionary is native unsigned integer type
Add setting check_dictionary_primary_key for compatibility(set check_dictionary_primary_key =false to disable checking)

close #48334

@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-improvement Pull request with some product improvements label Apr 3, 2023
@CurtizJ CurtizJ self-assigned this Apr 3, 2023
@CurtizJ CurtizJ added the can be tested Allows running workflows for external contributors label Apr 3, 2023
CurtizJ
CurtizJ reviewed Apr 3, 2023
View reviewed changes
src/Core/Settings.h Outdated
@@ -944,6 +944,7 @@ class IColumn;
M(Bool, regexp_dict_allow_hyperscan, true, "Allow regexp_tree dictionary using Hyperscan library.", 0) \
\
M(Bool, dictionary_use_async_executor, false, "Execute a pipeline for reading from a dictionary with several threads. It's supported only by DIRECT dictionary with CLICKHOUSE source.", 0) \
M(Bool, check_sample_dict_key_is_correct, true, "Check primary key type for simple dictionary is Native unsigned integer", 0) \
Copy link
Member

Choose a reason for hiding this comment

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

I think, check_dictionary_primary_key is a better name for the setting because it's actually a primary key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check_dictionary_primary_key is better, i will change it.

src/Dictionaries/getDictionaryConfigurationFromAST.cpp Outdated Show resolved Hide resolved
@lzydmxy
Copy link
Contributor Author

lzydmxy commented Apr 6, 2023

@CurtizJ Failed at some tests, and should I update those tests?

@CurtizJ
Copy link
Member

CurtizJ commented Apr 6, 2023

Yes, please update failed tests.

@lzydmxy
Copy link
Contributor Author

lzydmxy commented Apr 7, 2023

Yes, please update failed tests.

I update those failed Functional Tests: 02391_hashed_dictionary_shards 02364_dictionary_datetime_64_attribute_crash 01160_table_dependencies 02390_prometheus_ClickHouseStatusInfo_DictionaryStatus 02525_range_hashed_dictionary_update_field 01043_dictionary_attribute_properties_values 01042_system_reload_dictionary_reloads_completely

I also check those Integration Tests: test_backup_restore_new test_backup_restore_on_cluster test_backward_compatibility test_dictionaries_access test_dictionaries_ddl test_dictionaries_dependency test_dictionaries_dependency_xml test_dictionaries_mysql test_dictionaries_postgresql test_dictionaries_redis test_dictionary_ddl_on_cluster test_grant_and_revoke test_library_bridge test_mask_sensitive_info test_range_hashed_dictionary_types test_replicated_database
And update those Integration Tests: test_backup_restore_new test_backup_restore_on_cluster test_backward_compatibility test_dictionaries_access test_dictionaries_ddl test_dictionaries_dependency_xml test_mask_sensitive_info test_replicated_database

@lzydmxy lzydmxy changed the title Check sample dictionary key is native unsigned integer Check simple dictionary key is native unsigned integer Apr 10, 2023
@lzydmxy
Copy link
Contributor Author

lzydmxy commented Apr 11, 2023

@CurtizJ Hi, I need your help. Task UpgradeCheckMsan failed, how to deal with it？It seem like I can add config in path tests/config/users.d, but I'm not sure~

@CurtizJ
Copy link
Member

CurtizJ commented Apr 12, 2023

I'm not 100% sure but looks like it runs queries from the previous release on build from current release and since there were no setting the test fails. It should ok on next PRs after this one will be merged.

@CurtizJ CurtizJ merged commit 1520f3e into ClickHouse:master Apr 12, 2023
143 of 145 checks passed
@lzydmxy
Copy link
Contributor Author

lzydmxy commented Apr 12, 2023

I'm not 100% sure but looks like it runs queries from the previous release on build from current release and since there were no setting the test fails. It should ok on next PRs after this one will be merged.

Thx for you explanation : )

@tavplubix
Copy link
Member

@CurtizJ, you forgot to check the failed tests before merging. I have to revert this PR because it breaks tests.

@tavplubix tavplubix mentioned this pull request Apr 12, 2023
Merged
@lzydmxy
Copy link
Contributor Author

lzydmxy commented Apr 13, 2023

@CurtizJ, you forgot to check the failed tests before merging. I have to revert this PR because it breaks tests.

@tavplubix Task UpgradeCheck failed, it's any idea to fix it. Those tests should set check_dictionary_primary_key = 0 for compatibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CurtizJ CurtizJ CurtizJ approved these changes

Assignees

@CurtizJ CurtizJ

Labels
can be tested Allows running workflows for external contributors pr-improvement Pull request with some product improvements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Check primary key type for simple dictionary, to avoid misuse of dictionary tables
4 participants
@lzydmxy @CurtizJ @tavplubix @robot-ch-test-poll2