Skip to content

Disable use_skip_indexes_on_data_read by default#86273

Merged
CurtizJ merged 2 commits intoClickHouse:masterfrom
amosbird:disable-apply-skip-index-on-reading
Sep 1, 2025
Merged

Disable use_skip_indexes_on_data_read by default#86273
CurtizJ merged 2 commits intoClickHouse:masterfrom
amosbird:disable-apply-skip-index-on-reading

Conversation

@amosbird
Copy link
Copy Markdown
Collaborator

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

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

Disable use_skip_indexes_on_data_read by default. #81526 is merged with this setting enabled to do more thorough testing. cc @CurtizJ

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Aug 27, 2025

Workflow [PR], commit [0cd23dc]

Summary:

job_name test_name status info comment
Stateless tests (amd_debug, distributed plan, s3 storage, parallel) failure
01086_odbc_roundtrip FAIL

@CurtizJ CurtizJ self-assigned this Aug 27, 2025
@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Aug 27, 2025
@amosbird amosbird force-pushed the disable-apply-skip-index-on-reading branch from c435af6 to 0cd23dc Compare August 27, 2025 15:47
@CurtizJ CurtizJ added this pull request to the merge queue Sep 1, 2025
Merged via the queue into ClickHouse:master with commit 792a16a Sep 1, 2025
121 of 122 checks passed
@robot-ch-test-poll robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Sep 1, 2025
@rschu1ze
Copy link
Copy Markdown
Member

rschu1ze commented Sep 8, 2025

@amosbird @CurtizJ I am not aware of major issues with skip-index-lookup-during-read. Should we default-enable the setting by default?

@amosbird
Copy link
Copy Markdown
Collaborator Author

amosbird commented Sep 8, 2025

This feature itself doesn’t have correctness issues, but it’s not strictly better in all aspects. For example, applying filters during the plan stage can sometimes produce a smaller and more efficient execution plan. Also, it may affect some use cases that rely on explain estimate.

@alexey-milovidov
Copy link
Copy Markdown
Member

What are the next steps? Maybe enable in 25.10?

@amosbird
Copy link
Copy Markdown
Collaborator Author

What are the next steps? Maybe enable in 25.10?

I’m happy either way. I’ll go with your judgment on whether to enable it.

@rschu1ze
Copy link
Copy Markdown
Member

I’m happy either way. I’ll go with your judgment on whether to enable it.

Let's do it, this will also help to figure out edge cases.

(Tales from a stupid: I actually ran ClickBench with use_skip_indexes_on_data_read enabled vs. disabled. There was no difference and I realized that ClickBench doesn't define skip indexes :-) )

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Sep 17, 2025 via email

@rschu1ze rschu1ze changed the title Disable use_skip_indexes_on_data_read by default Disable use_skip_indexes_on_data_read by default Sep 21, 2025
@rschu1ze
Copy link
Copy Markdown
Member

I’m happy either way. I’ll go with your judgment on whether to enable it.

--> #87368

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

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants