Fix bad creation of sparse columns during mutation#92860
Fix bad creation of sparse columns during mutation#92860Avogar merged 5 commits intoClickHouse:masterfrom
Conversation
|
Workflow [PR], commit [a663c06] Summary: ❌
|
tests/queries/0_stateless/03774_wide_part_mutation_sparse_ratio_setting_bug.sql
Outdated
Show resolved
Hide resolved
…o_setting_bug.sql Co-authored-by: Azat Khuzhin <a3at.mail@gmail.com>
| settings = serialization_infos.getSettings(); | ||
| settings = SerializationInfo::Settings | ||
| { | ||
| (*source_part->storage.getSettings())[MergeTreeSetting::ratio_of_defaults_for_sparse_serialization], |
There was a problem hiding this comment.
Also why serialization_infos.getSettings().ratio_of_defaults_for_sparse initialized with default value (1)? Maybe this should be fixed instead?
There was a problem hiding this comment.
Also why serialization_infos.getSettings().ratio_of_defaults_for_sparse initialized with default value (1)?
I think to disable Sparse serialization if default settings are used. Maybe just for historical reasons.
Maybe this should be fixed instead?
The main thing here is that serialization_infos.getSettings().ratio_of_defaults_for_sparse will be default always, because we don't serialize this value in serializations.json and during deserialization of it from source part we always use default value. So we need to use value from storage settings always.
There was a problem hiding this comment.
Thanks, make sense.
Maybe it make sense to initialize this value from storage for now?
There was a problem hiding this comment.
But for bug-fix better to keep patches short, so LGTM
There was a problem hiding this comment.
Maybe it make sense to initialize this value from storage for now?
What do you mean? Propagate value from storage to SerializationInfo deserialization, so we initialize it always with the correct value?
There was a problem hiding this comment.
Propagate value from storage to SerializationInfo deserialization, so we initialize it always with the correct value?
Yes
There was a problem hiding this comment.
Well, I am not sure about it. ratio_of_defaults_for_sparse is used only in serialization, not deserialization. And before serialization we usually set fresh values for all settings from the storage settings (as it was here before and as it's done for merge and new parts creation). So ideally we should not use the value of ratio_of_defaults_for_sparse from deserialized SerializationInfo. It was used here because of my mistake only.
f89fa69
Cherry pick #92860 to 25.10: Fix bad creation of sparse columns during mutation in Wide part
…utation in Wide part
Cherry pick #92860 to 25.11: Fix bad creation of sparse columns during mutation in Wide part
…utation in Wide part
Cherry pick #92860 to 25.12: Fix bad creation of sparse columns during mutation in Wide part
…utation in Wide part
Backport #92860 to 25.12: Fix bad creation of sparse columns during mutation in Wide part
|
Hm, but it can be not only wide parts right? For instance indexes stored as a separate column in compact parts, so after |
Sounds like yes |
|
Actually, this is incorrect fix. It just hides the real bug. And #92419 didn't introduce it, it just uncovered it using the test https://fiddle.clickhouse.com/e89aeaeb-200d-45c6-9649-e8d70ca2dc71 The problem is: if updated column in source part is Sparse, but in the mutated part is not Sparse (for example due to changed setting to |
Backport #92860 to 25.11: Fix bad creation of sparse columns during mutation in Wide part
Backport #92860 to 25.10: Fix bad creation of sparse columns during mutation in Wide part
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix possible error
FILE_DOESNT_EXISTafter mutation of a sparse column withratio_of_defaults_for_sparse_serialization=0.0. Closes #92633Documentation entry for user-facing changes