Skip to content

Fix explain indexes with use_skip_indexes_on_data_read + use_query_condition_cache#88504

Merged
shankar-iyer merged 18 commits intoClickHouse:masterfrom
shankar-iyer:fix_explain_plan_with_qcc
Oct 31, 2025
Merged

Fix explain indexes with use_skip_indexes_on_data_read + use_query_condition_cache#88504
shankar-iyer merged 18 commits intoClickHouse:masterfrom
shankar-iyer:fix_explain_plan_with_qcc

Conversation

@shankar-iyer
Copy link
Copy Markdown
Member

@shankar-iyer shankar-iyer commented Oct 14, 2025

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Details

Fixes explain indexes with use_skip_indexes_on_data_read + use_query_condition_cache. Behavior change introduced and fixed in 25.10.

Resolves #88467

@shankar-iyer shankar-iyer marked this pull request as draft October 14, 2025 11:03
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Oct 14, 2025

Workflow [PR], commit [d3a0ede]

Summary:

job_name test_name status info comment
Integration tests (amd_binary, 1/5) failure
test_storage_s3_queue/test_5.py::test_migration[1-] FAIL cidb, flaky

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Oct 14, 2025
}

const bool use_skip_indexes_on_data_read = settings[Setting::use_skip_indexes_on_data_read] && !is_parallel_reading_from_replicas;
const bool use_skip_indexes_on_data_read = settings[Setting::use_skip_indexes_on_data_read] && !is_parallel_reading_from_replicas &&
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, why do we need l. 747?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Safe use of skip indexes in FINAL queries with use_skip_indexes_if_final_exact_mode=1 requires an additional PrimaryKeyExpand step and that step has to be done during full index analysis and cannot be postponed to data read time.

@rschu1ze rschu1ze self-assigned this Oct 15, 2025
@shankar-iyer
Copy link
Copy Markdown
Member Author

For Reference : 4 issues were found when testing this PR

  1. Issue with full-text index and use_skip_indexes_on_data_read=1, use_query_condition_cache=1 : Fix reading from text index with query condition cache #88660
  2. Mutations & skip indexes : Fix skip indexes on data read with patch parts #88702
  3. Queries with explicit max_rows_to_read andread_overflow_mode=throw could fail : Fixed in this PR
  4. Join order planning gets impacted because rows/granules pruning has been delayed with use_skip_indexes_on_data_read=1 : TODO

Enabling use_skip_indexes_on_data_read by default is being roll'ed back in #88638

@rschu1ze rschu1ze added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Oct 21, 2025
if (is_parallel_reading_from_replicas)
return false;

if (settings[Setting::read_overflow_mode] == OverflowMode::THROW && settings[Setting::max_rows_to_read]) /// Need to do full index analysis to get row count estimate
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This deserves a longer comment.

Maybe:

/// Settings `read_overflow_mode = 'throw'` and `max_rows_to_read` are evaluated early during execution, during initialization of the pipeline based on estimated row counts. Estimation doesn't work properly if the skip index is evaluated during data read (scan).

if (is_final_query && settings[Setting::use_skip_indexes_if_final_exact_mode])
return false;

if (settings[Setting::read_overflow_mode] == OverflowMode::THROW && settings[Setting::max_rows_to_read]) /// Need to do full index analysis to get row count estimate
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Needs a similarly long comment as above, see my proposal in the other comment.

@rschu1ze rschu1ze marked this pull request as ready for review October 30, 2025 13:09
@shankar-iyer
Copy link
Copy Markdown
Member Author

test_storage_s3_queue/test_5.py::test_migration failure is unrelated. There is one job in private CI that is not finishing, checking that.

@shankar-iyer shankar-iyer added this pull request to the merge queue Oct 31, 2025
Merged via the queue into ClickHouse:master with commit c4d8c13 Oct 31, 2025
122 of 124 checks passed
@shankar-iyer shankar-iyer deleted the fix_explain_plan_with_qcc branch October 31, 2025 11:45
@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-synced-to-cloud The PR is synced to the cloud repo label Oct 31, 2025
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added pr-backports-created-cloud deprecated label, NOOP pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR labels Oct 31, 2025
robot-clickhouse added a commit that referenced this pull request Oct 31, 2025
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Oct 31, 2025
clickhouse-gh bot added a commit that referenced this pull request Oct 31, 2025
Backport #88504 to 25.10: Fix explain indexes with use_skip_indexes_on_data_read + use_query_condition_cache
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud deprecated label, NOOP pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR 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.

Fix EXPLAIN PLAN indexes = 1 in ClickHouse >= 25.9

4 participants