Skip to content

add allow_part_offset_column_in_projections setting #85372

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
Merged
merged 8 commits into from
Aug 13, 2025
Merged

add allow_part_offset_column_in_projections setting #85372

merged 8 commits into from
Aug 13, 2025

Conversation

CheSema
Copy link
Member

@CheSema CheSema commented Aug 11, 2025 •
edited
Loading

I want to make allow_part_offset_column_in_projections=0 for 25.6 and allow_part_offset_column_in_projections=1 for newer releases.

release 25.6 has a bug #85314
The fix is big and it is not backported.

Changelog category (leave one):

  • Critical Bug Fix (crash, data loss, RBAC) or LOGICAL_ERROR

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

forbid using _part_offset column in projection in releases until it is stabilized.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh ClickHouse-GH
Copy link

clickhouse-gh bot commented Aug 11, 2025 •
edited
Loading

Workflow [PR], commit [c30f27a]

Summary: ❌

job_name test_name status info comment
Integration tests (amd_tsan, 6/6) failure
test_storage_kafka/test_batch_slow_4.py::test_kafka_consumer_failover[generate_new_create_table_query] FAIL
Stress test (amd_tsan) failure
Server died FAIL
Hung check failed, possible deadlock found (see hung_check.log) FAIL
Killed by signal (in clickhouse-server.log) FAIL
Fatal message in clickhouse-server.log (see fatal_messages.txt) FAIL
Killed by signal (output files) FAIL
Found signal in gdb.log FAIL
Upgrade check (amd_debug) failure
S3_ERROR No such key thrown (see clickhouse-server.log or no_such_key_errors.txt) FAIL

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Aug 11, 2025
@amosbird
Copy link
Collaborator

If we truly need this guard, it would be better implemented as a MergeTree setting, similar to allow_nullable_key.

@CheSema CheSema changed the title add allow_projection_with_parent_part_offset setting add allow_part_offset_column_in_projections setting Aug 12, 2025
@CheSema CheSema marked this pull request as ready for review August 12, 2025 12:30
@CheSema CheSema mentioned this pull request Aug 12, 2025
Closed
@Michicosun Michicosun self-assigned this Aug 13, 2025
Michicosun
Michicosun reviewed Aug 13, 2025
View reviewed changes
src/Storages/MergeTree/MergeTreeData.cpp Outdated
{
if (projection.with_parent_part_offset)
{
if (!(*getSettings())[MergeTreeSetting::allow_part_offset_column_in_projections])
Copy link
Member

Choose a reason for hiding this comment

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

this should also check projections for new_metadata

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, you are right. I have added a test for that.

@clickhouse-gh clickhouse-gh bot added pr-critical-bugfix pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-cloud and removed pr-not-for-changelog This PR should not be mentioned in the changelog labels Aug 13, 2025
@CheSema CheSema added this pull request to the merge queue Aug 13, 2025
Merged via the queue into master with commit cea91ea Aug 13, 2025
119 of 123 checks passed
@CheSema CheSema deleted the chesema-forbid-part-offset-in-proj branch August 13, 2025 17:35
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Aug 13, 2025
@robot-ch-test-poll2 robot-ch-test-poll2 added pr-backports-created-cloud pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR labels Aug 13, 2025
This was referenced Aug 13, 2025
Closed
Closed
This was referenced Aug 13, 2025
Merged
Merged
Merged
CheSema added a commit that referenced this pull request Aug 14, 2025
@CheSema
Merge pull request #85567 from ClickHouse/cherrypick/25.5/85372
2e82bf7 
Cherry pick #85372 to 25.5: add allow_part_offset_column_in_projections setting
CheSema added a commit that referenced this pull request Aug 14, 2025
@CheSema
Merge pull request #85568 from ClickHouse/cherrypick/25.6/85372
3953e7f 
Cherry pick #85372 to 25.6: add allow_part_offset_column_in_projections setting
CheSema added a commit that referenced this pull request Aug 14, 2025
@CheSema
Merge pull request #85569 from ClickHouse/cherrypick/25.7/85372
6d488c6 
Cherry pick #85372 to 25.7: add allow_part_offset_column_in_projections setting
robot-clickhouse added a commit that referenced this pull request Aug 14, 2025
@robot-clickhouse
Backport #85372 to 25.5: add allow_part_offset_column_in_projections …
e111180 
…setting
@robot-ch-test-poll3 robot-ch-test-poll3 mentioned this pull request Aug 14, 2025
Closed
robot-clickhouse added a commit that referenced this pull request Aug 14, 2025
@robot-clickhouse
Backport #85372 to 25.6: add allow_part_offset_column_in_projections …
a7b0f31 
…setting
@robot-ch-test-poll3 robot-ch-test-poll3 mentioned this pull request Aug 14, 2025
Closed
robot-clickhouse added a commit that referenced this pull request Aug 14, 2025
@robot-clickhouse
Backport #85372 to 25.7: add allow_part_offset_column_in_projections …
72a82bf 
…setting
@robot-ch-test-poll3 robot-ch-test-poll3 mentioned this pull request Aug 14, 2025
Closed
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Aug 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Michicosun Michicosun Michicosun approved these changes

Assignees

@Michicosun Michicosun

Labels
pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud pr-critical-bugfix pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-cloud pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-synced-to-cloud The PR is synced to the cloud repo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@CheSema @amosbird @Michicosun @robot-ch-test-poll1 @robot-ch-test-poll2 @robot-clickhouse-ci-2