Fix on-disk format breakage for secondary indices over Nullable column

[1] breaks on disk format (and the relevant change in the:

  [1]: https://github.com/ClickHouse/ClickHouse/pull/12455#discussion_r682830812

Too bad that I checked this patchset only for compatibility after
reverting this patch [2] (use case: I've applied it manually, then
revert it, and data skipping indexes over Nullable column had been
broken)

  [2]: https://github.com/ClickHouse/ClickHouse/pull/12455#issuecomment-823423772

But this patchset actually breaks compatibility with older versions of
clickhouse for Nullable data skipping indexes after simple upgrade:

Here is a simple reproducer:

    --
    -- run this with 21.6 or similar (i.e. w/o this patch)
    --

    CREATE TABLE data
    (
        `key` Int,
        `value` Nullable(Int),
        INDEX value_index value TYPE minmax GRANULARITY 1
    )
    ENGINE = MergeTree
    ORDER BY key;

    INSERT INTO data SELECT
        number,
        number
    FROM numbers(10000);

    SELECT * FROM data WHERE value = 20000 SETTINGS force_data_skipping_indices = 'value_index' SETTINGS force_data_skipping_indices = 'value_index', max_rows_to_read=1;

Now upgrade and run the query again:

    SELECT * FROM data WHERE value = 20000 SETTINGS force_data_skipping_indices = 'value_index' SETTINGS force_data_skipping_indices = 'value_index', max_rows_to_read=1;

And it will fail because of on disk format changes:

    $ ll --time-style=+ data/*/data/all_1_1_0/skp*.idx
    -rw-r----- 1 azat azat 36  data/with_nullable_patch/data/all_1_1_0/skp_idx_value_index.idx
    -rw-r----- 1 azat azat 37  data/without_nullable_patch/data/all_1_1_0/skp_idx_value_index.idx

    $ md5sum data/*/data/all_1_1_0/skp*.idx
    a19c95c4a14506c65665a1e30ab404bf  data/with_nullable_patch/data/all_1_1_0/skp_idx_value_index.idx
    e50e2fcfa873b232196623d56ab26105  data/without_nullable_patch/data/all_1_1_0/skp_idx_value_index.idx

Note, that there is no stable release with this patch included yet, so
no need to backport.

Also note that you may create data skipping indexes over Nullable
column even before [3].

  [3]: https://github.com/ClickHouse/ClickHouse/pull/12455

v2: break cases when granulas has Null in values due to backward
compatibility
This commit is contained in:
Azat Khuzhin 2021-08-04 21:25:51 +03:00
parent f710f1e954
commit dee27fcbb9
2 changed files with 48 additions and 11 deletions

View File

@ -39,9 +39,26 @@ void MergeTreeIndexGranuleMinMax::serializeBinary(WriteBuffer & ostr) const
{ {
const DataTypePtr & type = index_sample_block.getByPosition(i).type; const DataTypePtr & type = index_sample_block.getByPosition(i).type;
auto serialization = type->getDefaultSerialization(); auto serialization = type->getDefaultSerialization();
if (!type->isNullable())
{
serialization->serializeBinary(hyperrectangle[i].left, ostr); serialization->serializeBinary(hyperrectangle[i].left, ostr);
serialization->serializeBinary(hyperrectangle[i].right, ostr); serialization->serializeBinary(hyperrectangle[i].right, ostr);
} }
else
{
/// NOTE: that this serialization differs from
/// IMergeTreeDataPart::MinMaxIndex::store() due to preserve
/// backward compatibility.
bool is_null = hyperrectangle[i].left.isNull() || hyperrectangle[i].right.isNull(); // one is enough
writeBinary(is_null, ostr);
if (!is_null)
{
serialization->serializeBinary(hyperrectangle[i].left, ostr);
serialization->serializeBinary(hyperrectangle[i].right, ostr);
}
}
}
} }
void MergeTreeIndexGranuleMinMax::deserializeBinary(ReadBuffer & istr) void MergeTreeIndexGranuleMinMax::deserializeBinary(ReadBuffer & istr)
@ -54,14 +71,30 @@ void MergeTreeIndexGranuleMinMax::deserializeBinary(ReadBuffer & istr)
{ {
const DataTypePtr & type = index_sample_block.getByPosition(i).type; const DataTypePtr & type = index_sample_block.getByPosition(i).type;
auto serialization = type->getDefaultSerialization(); auto serialization = type->getDefaultSerialization();
if (!type->isNullable())
{
serialization->deserializeBinary(min_val, istr); serialization->deserializeBinary(min_val, istr);
serialization->deserializeBinary(max_val, istr); serialization->deserializeBinary(max_val, istr);
}
// NULL_LAST else
if (min_val.isNull()) {
min_val = PositiveInfinity(); /// NOTE: that this serialization differs from
if (max_val.isNull()) /// IMergeTreeDataPart::MinMaxIndex::load() due to preserve
max_val = PositiveInfinity(); /// backward compatibility.
bool is_null;
readBinary(is_null, istr);
if (!is_null)
{
serialization->deserializeBinary(min_val, istr);
serialization->deserializeBinary(max_val, istr);
}
else
{
min_val = Null();
max_val = Null();
}
}
hyperrectangle.emplace_back(min_val, true, max_val, true); hyperrectangle.emplace_back(min_val, true, max_val, true);
} }
} }

View File

@ -49,11 +49,15 @@ SET force_primary_key = 0;
SELECT * FROM nullable_minmax_index ORDER BY k; SELECT * FROM nullable_minmax_index ORDER BY k;
SET max_rows_to_read = 6; SET max_rows_to_read = 6;
SELECT * FROM nullable_minmax_index WHERE v IS NULL; SELECT * FROM nullable_minmax_index WHERE v IS NULL;
SET max_rows_to_read = 8; -- NOTE: granuals with Null values cannot be filtred in data skipping indexes,
-- due to backward compatibility
SET max_rows_to_read = 0;
SELECT * FROM nullable_minmax_index WHERE v IS NOT NULL; SELECT * FROM nullable_minmax_index WHERE v IS NOT NULL;
SET max_rows_to_read = 6; SET max_rows_to_read = 6;
SELECT * FROM nullable_minmax_index WHERE v > 2; SELECT * FROM nullable_minmax_index WHERE v > 2;
SET max_rows_to_read = 4; -- NOTE: granuals with Null values cannot be filtred in data skipping indexes,
-- due to backward compatibility
SET max_rows_to_read = 0;
SELECT * FROM nullable_minmax_index WHERE v <= 2; SELECT * FROM nullable_minmax_index WHERE v <= 2;
DROP TABLE nullable_key; DROP TABLE nullable_key;