Skip to content

Fix usage of non-materialized skip indexes#32359

Merged
CurtizJ merged 3 commits intoClickHouse:masterfrom
CurtizJ:fix-non-materialized-skip-index
Dec 13, 2021
Merged

Fix usage of non-materialized skip indexes#32359
CurtizJ merged 3 commits intoClickHouse:masterfrom
CurtizJ:fix-non-materialized-skip-index

Conversation

@CurtizJ
Copy link
Copy Markdown
Member

@CurtizJ CurtizJ commented Dec 7, 2021 •
edited by alexey-milovidov
Loading

Changelog category (leave one):

  • Bug Fix (user-visible misbehaviour in official stable or prestable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix failures in queries that are trying to use skipping indices, which are not materialized yet. Fixes #32292 and #30343.

@robot-clickhouse robot-clickhouse added the pr-bugfix Pull request with bugfix, not backported by default label Dec 7, 2021
azat
azat reviewed Dec 8, 2021
View reviewed changes
tests/queries/0_stateless/02131_skip_index_not_materialized.sql Outdated

INSERT INTO t_index_non_materialized VALUES (1);

ALTER TABLE t_index_non_materialized ADD INDEX ind (a) TYPE set(1) GRANULARITY 1;
Copy link
Copy Markdown
Member

@azat azat Dec 8, 2021

Choose a reason for hiding this comment

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

Nice catch! Worth adding a check for minmax index too? (since it is a separate code branch)

Copy link
Copy Markdown
Member Author

@CurtizJ CurtizJ Dec 8, 2021

Choose a reason for hiding this comment

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

There already exists a check

ClickHouse/src/Storages/MergeTree/MergeTreeIndexMinMax.cpp

Lines 216 to 223 in 4e6bf24

MergeTreeIndexFormat MergeTreeIndexMinMax::getDeserializedFormat(const DiskPtr disk, const std::string & relative_path_prefix) const
{
if (disk->exists(relative_path_prefix + ".idx2"))
return {2, ".idx2"};
else if (disk->exists(relative_path_prefix + ".idx"))
return {1, ".idx"};
return {0 /* unknown */, ""};
}

Added it to test.

Copy link
Copy Markdown
Member

@azat azat Dec 8, 2021

Choose a reason for hiding this comment

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

There already exists a check

Yep, I meant adding it to test (since those branches can be modified independently)

@Zhile
Copy link
Copy Markdown

Zhile commented Dec 8, 2021

Please help to backport this fix to 21.11. Thanks.

@kitaisreal kitaisreal self-assigned this Dec 10, 2021
@Zhile
Copy link
Copy Markdown

Zhile commented Dec 13, 2021

@CurtizJ can this be merged？

@Zhile Zhile mentioned this pull request Dec 13, 2021
Closed
@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Dec 13, 2021

@Mergifyio update

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Dec 13, 2021

update

✅ Branch has been successfully updated

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Dec 13, 2021

There are failing tests. This PR will be merged after either author (@CurtizJ) or reviewer (@kitaisreal) will check and investigate the failures of the tests. If PR has failing tests, it cannot be merged as is.

@CurtizJ CurtizJ merged commit a241103 into ClickHouse:master Dec 13, 2021
@CurtizJ
Copy link
Copy Markdown
Member Author

CurtizJ commented Dec 13, 2021

Stress test: Logical error: 'Chunk should have AggregatedChunkInfo in GroupingAggregatedTransform.'. #30692

This was referenced Dec 13, 2021
Closed
Merged
Closed
Merged
Merged
Merged
robot-clickhouse pushed a commit that referenced this pull request Dec 13, 2021
Backport #32359 to 21.10: Fix usage of non-materialized skip indexes
d762703
@robot-clickhouse robot-clickhouse mentioned this pull request Dec 13, 2021
Merged
robot-clickhouse pushed a commit that referenced this pull request Dec 13, 2021
Backport #32359 to 21.12: Fix usage of non-materialized skip indexes
d92dc59
@robot-clickhouse robot-clickhouse mentioned this pull request Dec 13, 2021
Merged
robot-clickhouse pushed a commit that referenced this pull request Dec 13, 2021
Backport #32359 to 21.11: Fix usage of non-materialized skip indexes
e351ecd
@robot-clickhouse robot-clickhouse mentioned this pull request Dec 13, 2021
Merged
robot-clickhouse pushed a commit that referenced this pull request Dec 13, 2021
Backport #32359 to 21.9: Fix usage of non-materialized skip indexes
a8cd6fd
@robot-clickhouse robot-clickhouse mentioned this pull request Dec 13, 2021
Merged
@CurtizJ CurtizJ mentioned this pull request Dec 14, 2021
Closed
CurtizJ added a commit that referenced this pull request Dec 14, 2021
@CurtizJ
Merge pull request #32712 from ClickHouse/backport/21.9/32359
6a734ad 
Backport #32359 to 21.9: Fix usage of non-materialized skip indexes
CurtizJ added a commit that referenced this pull request Dec 14, 2021
@CurtizJ
Merge pull request #32711 from ClickHouse/backport/21.11/32359
2cb868a 
Backport #32359 to 21.11: Fix usage of non-materialized skip indexes
CurtizJ added a commit that referenced this pull request Dec 14, 2021
@CurtizJ
Merge pull request #32710 from ClickHouse/backport/21.12/32359
1a21b59 
Backport #32359 to 21.12: Fix usage of non-materialized skip indexes
CurtizJ added a commit that referenced this pull request Dec 14, 2021
@CurtizJ
Merge pull request #32709 from ClickHouse/backport/21.10/32359
cf2ba3e 
Backport #32359 to 21.10: Fix usage of non-materialized skip indexes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@azat azat azat left review comments

+1 more reviewer

@kitaisreal kitaisreal kitaisreal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@kitaisreal kitaisreal

Labels

pr-bugfix Pull request with bugfix, not backported by default

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Query that uses skip index fails if index is not materialized

6 participants

@CurtizJ @Zhile @alexey-milovidov @azat @kitaisreal @robot-clickhouse