Fix possible clickhouse-local abort on JSONEachRow schema inference #46731
Conversation
robot-ch-test-poll1
added
the
pr-bugfix
|
Big diff is just a test file |
| for (auto & new_name_and_type : new_names_and_types) | ||
| { | ||
| auto & name = new_name_and_type.name; | ||
| auto & new_type = new_name_and_type.type; | ||
| if (names_set.contains(name)) |
There was a problem hiding this comment.
Is there really difference between these syntax versions?
There was a problem hiding this comment.
Seems like whil using for auto & [name, new_type] : new_names_and_types
name and new_type are not references but copied objects.
Maybe because we have only specialization for const const NameAndTypePair & here:
ClickHouse/src/Core/NamesAndTypes.h
Lines 53 to 62 in b0427c2
There was a problem hiding this comment.
I tried to add specialization for NameAndTypePair & name_and_type but it didn't help for some reason...
There was a problem hiding this comment.
And we want to change the new_type in chooseResultColumnType, ok.
Why do we need such a big test case for this, why it can't be reproduced with smaller one?
There was a problem hiding this comment.
It was reproduced only on large file. But I just realized that with address sanitizer it will reproduce with 1 row. Let me update the test
There was a problem hiding this comment.
A Fuzzer of data formats will be helpful.
There was a problem hiding this comment.
I tried to add specialization for NameAndTypePair & name_and_type but it didn't help for some reason...
To make it work you have to make sure that:
- Your return type is reference e.g.
return std::ref(name_and_type.name);(or just avoid usingdecltype(auto)) - Tuple also holds reference, not value
template <> struct tuple_element<0, DB::NameAndTypePair> { using type = DB::String&; };
There was a problem hiding this comment.
I've found a better solution. You can actually keep tuple of values (as it is now), but just change return type from decltype(auto) to std::tuple_element_t<I, NameAndTypePair>& or auto& and of course provide non-const overload.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix possible clickhouse-local abort on JSONEachRow schema inference